LinuxLists.cc - [PATCH] ftrace: introduce tracing_reset_online

[permalink] [raw]

Subject: Re: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper

* Pekka J Enberg <[email protected]> wrote:

> From: Pekka Enberg <[email protected]>
>
> This patch factors out common code from multiple tracers into a
> tracing_reset_online_cpus() function and converts the tracers to use it.
>
> Signed-off-by: Pekka Enberg <[email protected]>
> ---
> kernel/trace/trace.c | 10 ++++++++++
> kernel/trace/trace.h | 1 +
> kernel/trace/trace_boot.c | 12 +-----------
> kernel/trace/trace_functions.c | 14 ++------------
> kernel/trace/trace_hw_branches.c | 14 ++------------
> kernel/trace/trace_mmiotrace.c | 6 +-----
> kernel/trace/trace_sched_switch.c | 14 ++------------
> kernel/trace/trace_sysprof.c | 12 +-----------
> 8 files changed, 20 insertions(+), 63 deletions(-)

looks like a nice simplification! Applied to tip/tracing/ftrace, thanks
Pekka!

Ingo

2008-12-19 17:20:58

[permalink] [raw]

Subject: Re: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper

On Fri, 19 Dec 2008, Fr?d?ric Weisbecker wrote:
>
>
> That's looks good.
> 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.

Now we have recently added /debug/tracing/tracing_on which can quickly
stop tracing. I may be able to use that, and we can let the
tracing_enable" reset it too.

I'll have to take a look at my scripts to see if that would work.

-- Steve

2008-12-19 22:12:46

[permalink] [raw]

Subject: Re: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper

* Steven Rostedt <[email protected]> wrote:

>
> On Fri, 19 Dec 2008, Fr?d?ric Weisbecker wrote:
> >
> >
> > That's looks good.
> > 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.
>
> Now we have recently added /debug/tracing/tracing_on which can quickly
> stop tracing. I may be able to use that, and we can let the
> tracing_enable" reset it too.
>
> I'll have to take a look at my scripts to see if that would work.

yep, using /debug/tracing/tracing_enabled is the right model to quickly
toggle tracing without losing the buffer.

The 'switch tracer' op is really a slow thing anyway. It can involve
unpatching/repatching functions, etc. etc. - and while it works, it
shouldnt really be encouraged as a dynamic tool coding pattern.

Ingo

2008-12-19 22:45:18

[permalink] [raw]

Subject: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

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.

On Fri, 19 Dec 2008 12:20:18 -0500 (EST)
Steven Rostedt <[email protected]> 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?

Or has something changed that I don't know about?

Also another important but not so related question:
are the ring buffers allocated always on boot, whenever any tracer
(say, only mmiotrace) is enabled in the kernel configuration?

The ring buffers are huge and eat a considerable chunk of precious RAM.
How could distributors ever enable mmiotrace in their kernel
configurations by default, if it eats lots of memory for nothing?

If distributors do not enable mmiotrace by default, we are in a worse
situation than with out-of-tree mmiotrace module (if it could work).
Users need to reconfigure and recompile their kernels, which is
something we wanted to avoid. This is the reality right now.

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?

> Now we have recently added /debug/tracing/tracing_on which can quickly
> stop tracing. I may be able to use that, and we can let the
> tracing_enable" reset it too.

Does this mean I have to implement yet another on/off hook?

IMHO it is starting to be confusing having all these
current_tracer, tracing_enabled, tracing_on etc.

> I'll have to take a look at my scripts to see if that would work.
>
> -- Steve

Thanks.

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-19 22:57:38

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

* Pekka Paalanen <[email protected]> wrote:

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

hm, that ftrace behavior is silly. Steve, i think i mentioned this a long
time ago and i thought it got fixed? Changing the ring buffer size is a
slow op, it should include an implicit reset and should be plug-and-play
with no dependencies of having to stop the trace or something.

Ingo

2008-12-19 23:27:52

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Fri, 19 Dec 2008, Ingo Molnar wrote:

>
> * Pekka Paalanen <[email protected]> wrote:
>
> > > 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?
>
> hm, that ftrace behavior is silly. Steve, i think i mentioned this a long
> time ago and i thought it got fixed? Changing the ring buffer size is a
> slow op, it should include an implicit reset and should be plug-and-play
> with no dependencies of having to stop the trace or something.

I think you are referencing a different issue, where we passed in a NULL
buffer pointer and expected it to be resized. I still think that should
not return a success, but that's a different issue altogether.

Before the ring buffer, the only safe way to resize the buffer was to
switch the tracer to none (aka nop). Now, the only thing you need to do
is disable tracing. This is probably a good thing since it forces the user
to stop tracing, otherwise the tracing will stop during the resize anyway,
and the user will wonder why they lost data.

Resizing is a dangerous operation to do while tracing. Right now ftrace
enforces the the protection of writes to the ring buffer during resize
(the ftrace infrastructure does, the plugins do not need to worry about
it).

Now that we have other ways to disable writing to the ring buffer, we can
move that down to the ring buffer layer. When the ring buffer was
originaly written, there was no such method to stop writing. What would
need to be done now, is the ring buffers would disable writing to all CPU
buffers, do a synchronize sched, since writing requires preemption
disabled. Then we would need to grab all the reader spin locks for each
cpu buffer (that will be ugly :-( ), and then we could safely change the
size of the buffers.

I could easily write this up. I'll see if I can get to it after I release
the -rt trees. But like I said, there will be time when tracing will be
stopped, and it would be a good idea to make the user stop it so they do
not get confused about why they are missing trace output after they did a
resize.

-- Steve

2008-12-19 23:35:20

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

* Steven Rostedt <[email protected]> wrote:

> > hm, that ftrace behavior is silly. Steve, i think i mentioned this a
> > long time ago and i thought it got fixed? Changing the ring buffer
> > size is a slow op, it should include an implicit reset and should be
> > plug-and-play with no dependencies of having to stop the trace or
> > something.
>
> I think you are referencing a different issue, where we passed in a NULL
> buffer pointer and expected it to be resized. I still think that should
> not return a success, but that's a different issue altogether.
>
> Before the ring buffer, the only safe way to resize the buffer was to
> switch the tracer to none (aka nop). Now, the only thing you need to do
> is disable tracing. This is probably a good thing since it forces the
> user to stop tracing, otherwise the tracing will stop during the resize
> anyway, and the user will wonder why they lost data.

uhm, the user just resized the buffer - he will not be wondering about any
impact.

And that's why we should do a full reset: that makes it abundantly clear
what happened. We shouldnt pretend to be able to do something (seemless,
lightweight buffer size change with no visible impact) which we obviously
cannot reliably offer.

Buffer resize is a slow op, and everyone realizes that. Most people will
in fact say: "hey, I didnt even need to reboot to change the trace buffer
size, cool!".

The current "only allow resizing while tracing is disabled" is an
unintuitive and counterproductive restriction - a borderline bug in fact.

Ingo

2008-12-20 00:03:57

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

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 <[email protected]> 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.

>
> Or has something changed that I don't know about?
>
> Also another important but not so related question:
> are the ring buffers allocated always on boot, whenever any tracer
> (say, only mmiotrace) is enabled in the kernel configuration?

Currently all tracer plugins share the same buffer. I've been thinking
about changing this so that each plugin has its own buffer. Then it could
allocate its buffer when it is enabled. This way, we could have
simultaneous traces. This is currently the reason why we only allow
one plugin at a time. But changing this would be a 2.6.30 feature ;-)

>
> The ring buffers are huge and eat a considerable chunk of precious RAM.
> How could distributors ever enable mmiotrace in their kernel
> configurations by default, if it eats lots of memory for nothing?

Hmm, good point. We could change the allocation to when it is first
enabled. Something other than 'nop' being put into the current_tracer
file.

>
> If distributors do not enable mmiotrace by default, we are in a worse
> situation than with out-of-tree mmiotrace module (if it could work).
> Users need to reconfigure and recompile their kernels, which is
> something we wanted to avoid. This is the reality right now.
>
> 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.

>
> > Now we have recently added /debug/tracing/tracing_on which can quickly
> > stop tracing. I may be able to use that, and we can let the
> > tracing_enable" reset it too.
>
> Does this mean I have to implement yet another on/off hook?

Nope, the on/off hook is extremely fast, and the plugins do not even know
when they happen. The on/off simply turns off writing to the ring buffer.
The plugin functions will still be called, it is just that they will fail
to write to the ring buffers. As stated above, the
ring_buffer_reserve_lock will return a NULL.

>
> IMHO it is starting to be confusing having all these
> current_tracer, tracing_enabled, tracing_on etc.

The problem with giving people the power of what they want, is that it
makes it complex ;-) Switching tracers is the slowest function, and
causes the full work of any initializations. This is where we could
allocate a separate ring buffer (in the future).

The tracing_enabled is the way to start and stop a trace. I'm considering
to implement Frederic's request to reset the buffer on enabled. This is
quick but requires locks and mutexes to be taken. It calls a hook to the
plugin because different plugins actually want to reset (the irq latency
tracers reset with this).

The tracing_on is something that has been asked by developers to give a
way to start and stop tracing fast as possible, with no mutexes or added
locks. In fact, this option is local to the ring buffer code. Ftrace does
not even use it directly. It just a global flag to stop tracing. There's
also an in-kernel equivalent tracing_on() and tracing_off(). This just
sets or clears a global flag that will stop any more writes to the trace
buffer.

Note, we also have an in-kernel version of tracing_enabled which is
tracing_start() and tracing_stop(). These are nested. That is, every
tracing_stop() must be accompanied by a tracing_start() otherwise you
leave the tracing disabled, and not even user space can start it. This
side effect is the way I disable tracing when a problem is detected.

The tracing_on() and tracing_off() is not nested. It is just like a light
switch. Two processes can call tracing_off() and one can call tracing_on()
and tracing will be enabled again. The kernel can call tracing_off() and
userspace can enable it by 'echo 1 > tracing_on'. This is actually a very
useful feature in debugging. When an anomaly happens, the kernel can call
tracing_off(), the user could examine the trace, and then enable tracing
again if they want to continue testing.

-- Steve

2008-12-20 00:29:41

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Sat, 20 Dec 2008, Ingo Molnar wrote:
>
> uhm, the user just resized the buffer - he will not be wondering about any
> impact.

I can imagine some users that would ;-)

>
> And that's why we should do a full reset: that makes it abundantly clear
> what happened. We shouldnt pretend to be able to do something (seemless,
> lightweight buffer size change with no visible impact) which we obviously
> cannot reliably offer.

Some tracers, namely the latency ones (irqsoff and friends, as well as the
sched wakeup) need to resize two buffers, and if this is done while
tracing is enabled, the special "switch" operation can cause problems.
Keeping tracing on while resizing can be done, but it needs to done
carefully.

>
> Buffer resize is a slow op, and everyone realizes that. Most people will
> in fact say: "hey, I didnt even need to reboot to change the trace buffer
> size, cool!".

Exactly! Currently they will be happy that they do not need to reboot.

>
> The current "only allow resizing while tracing is disabled" is an
> unintuitive and counterproductive restriction - a borderline bug in fact.

Having to disable tracing to resize is much better than having to reboot.

I'm not defending that this is the way to go. I never planned on keeping
this the defacto standard. I've been setting up infrastructure to allow
for a seamless resize. If you think we should reset the buffers on resize,
then that would be a great way for the user to know why they are missing
**all** their data.

What I'm trying to say is that the more we allow during resize, the more
likely there will be a critical bug that might lock up the system. The
whole point about writing the ring buffer was to do it incrementally.
Start out with being straight forward and simple, then we could add
features as we understand things better.

IOW, I totally agree that we should make it as intuitive as possible. But
this needs to be done step by step. I wrote 10 different versions of the
ring buffer in a week. Most of them were complete rewrites. I want this to
be as robust and powerful as you do, but I also want to be cautious about
doing things that might crash the system.

Ftrace already had the infrastructure in place to protect against resize.
I just used it to give me time to do other things with the ring buffer.
I always planned on having the resize protection back inside the ring
buffer code when I got around to it.

-- Steve

2008-12-20 01:38:39

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
Steven Rostedt <[email protected]> wrote:

> On Sat, 20 Dec 2008, Ingo Molnar wrote:
> >
> > The current "only allow resizing while tracing is disabled" is an
> > unintuitive and counterproductive restriction - a borderline bug in fact.
>
> Having to disable tracing to resize is much better than having to reboot.
>
> I'm not defending that this is the way to go. I never planned on keeping
> this the defacto standard. I've been setting up infrastructure to allow
> for a seamless resize. If you think we should reset the buffers on resize,
> then that would be a great way for the user to know why they are missing
> **all** their data.

I thought this was just about not having to do

$ echo 0 > tracing_enabled
$ echo 28764243 > buffer_size
$ echo 1 > tracing_enabled

and instead just do

$ echo 28764243 > buffer_size

which would do exactly the same, except being easier for the user.
Personally I've never dreamed of any kind of resize-in-flight.

> What I'm trying to say is that the more we allow during resize, the more
> likely there will be a critical bug that might lock up the system. The
> whole point about writing the ring buffer was to do it incrementally.
> Start out with being straight forward and simple, then we could add
> features as we understand things better.
>
> IOW, I totally agree that we should make it as intuitive as possible. But
> this needs to be done step by step. I wrote 10 different versions of the
> ring buffer in a week. Most of them were complete rewrites. I want this to
> be as robust and powerful as you do, but I also want to be cautious about
> doing things that might crash the system.
>
> Ftrace already had the infrastructure in place to protect against resize.
> I just used it to give me time to do other things with the ring buffer.
> I always planned on having the resize protection back inside the ring
> buffer code when I got around to it.
>
> -- Steve

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-20 01:46:48

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Sat, 20 Dec 2008, Pekka Paalanen wrote:

> On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
> Steven Rostedt <[email protected]> wrote:
>
> I thought this was just about not having to do
>
> $ echo 0 > tracing_enabled
> $ echo 28764243 > buffer_size
> $ echo 1 > tracing_enabled
>
> and instead just do
>
> $ echo 28764243 > buffer_size
>
> which would do exactly the same, except being easier for the user.
> Personally I've never dreamed of any kind of resize-in-flight.
>

To implement this at the ftrace level should be a trivial change. I'm just
saying that doing this at the "ring buffer" level might be a bit more
complex. The ring buffer has no idea of ftrace. It should not. It is at
a lower lever than ftrace. Although, I do think some of the protecting
that is done at the tracing level during resize should be moved down into
the ring buffer layer.

-- Steve

2008-12-20 02:18:19

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

Steven,

thank you for a complete reply. Few comments below.

On Fri, 19 Dec 2008 19:03:42 -0500 (EST)
Steven Rostedt <[email protected]> wrote:

> On Sat, 20 Dec 2008, Pekka Paalanen wrote:
>
> > 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 guess I don't really see what was so bad about switching to the "none"
tracer, as the resize operation is expected wipe everything anyway.
Likely because for mmiotrace, toggling tracing_enabled is the same as
switching tracers.

> 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.
>
> > The ring buffers are huge and eat a considerable chunk of precious RAM.
> > How could distributors ever enable mmiotrace in their kernel
> > configurations by default, if it eats lots of memory for nothing?
>
> Hmm, good point. We could change the allocation to when it is first
> enabled. Something other than 'nop' being put into the current_tracer
> file.

I'm very much looking forward to this.

> >
> > If distributors do not enable mmiotrace by default, we are in a worse
> > situation than with out-of-tree mmiotrace module (if it could work).
> > Users need to reconfigure and recompile their kernels, which is
> > something we wanted to avoid. This is the reality right now.
> >
> > 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.

The "nop" having no buffers at boot would be enough, but this would be
even better.

> >
> > > Now we have recently added /debug/tracing/tracing_on which can quickly
> > > stop tracing. I may be able to use that, and we can let the
> > > tracing_enable" reset it too.
> >
> > Does this mean I have to implement yet another on/off hook?
>
> Nope, the on/off hook is extremely fast, and the plugins do not even know
> when they happen. The on/off simply turns off writing to the ring buffer.
> The plugin functions will still be called, it is just that they will fail
> to write to the ring buffers. As stated above, the
> ring_buffer_reserve_lock will return a NULL.

Does this also increment the lost events counters?

> > IMHO it is starting to be confusing having all these
> > current_tracer, tracing_enabled, tracing_on etc.

>
> The tracing_enabled is the way to start and stop a trace. I'm considering
> to implement Frederic's request to reset the buffer on enabled. This is
> quick but requires locks and mutexes to be taken. It calls a hook to the
> plugin because different plugins actually want to reset (the irq latency
> tracers reset with this).
>
> The tracing_on is something that has been asked by developers to give a
> way to start and stop tracing fast as possible, with no mutexes or added
> locks. In fact, this option is local to the ring buffer code. Ftrace does
> not even use it directly. It just a global flag to stop tracing. There's
> also an in-kernel equivalent tracing_on() and tracing_off(). This just
> sets or clears a global flag that will stop any more writes to the trace
> buffer.

Why not call tracing_on, say, logging_enabled?
IMHO tracing_enabled vs. tracing_on is incomprehesible, but
tracing_enabled vs. logging_enabled is a little more understandable.
current_tracer is self-explanatory, and tracing_enabled used to be.

Thanks.

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-20 02:32:25

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Fri, 19 Dec 2008 20:46:38 -0500 (EST)
Steven Rostedt <[email protected]> wrote:

>
> On Sat, 20 Dec 2008, Pekka Paalanen wrote:
>
> > On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
> > Steven Rostedt <[email protected]> wrote:
> >
> > I thought this was just about not having to do
> >
> > $ echo 0 > tracing_enabled
> > $ echo 28764243 > buffer_size
> > $ echo 1 > tracing_enabled
> >
> > and instead just do
> >
> > $ echo 28764243 > buffer_size
> >
> > which would do exactly the same, except being easier for the user.
> > Personally I've never dreamed of any kind of resize-in-flight.
> >
>
> To implement this at the ftrace level should be a trivial change. I'm just
> saying that doing this at the "ring buffer" level might be a bit more
> complex. The ring buffer has no idea of ftrace. It should not. It is at
> a lower lever than ftrace. Although, I do think some of the protecting
> that is done at the tracing level during resize should be moved down into
> the ring buffer layer.

Aah, so you are saying that the buffer_size file (or whatever it was called)
is part of the ring buffer user API, and not tracing user API?

But the ring buffer is just a buffer, is it meaningful to adjust a ring
buffer size? I cannot tell tracing to go use a different buffer. And if
there will be other users of ring buffers, they would probably want to
have their own control over the buffer size.

As a user, I want to adjust *the* tracing ring buffer size, not some ring
buffer size.

Am I making any sense? I'm trying to say that in my opinion, the
buffer_size file does not belong to the "ring buffer" level. The upper
levels should decide whether and how it offers buffer resizing.

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-20 02:48:12

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Sat, 20 Dec 2008, Pekka Paalanen wrote:
> >
> > 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.
> >
> > > The ring buffers are huge and eat a considerable chunk of precious RAM.
> > > How could distributors ever enable mmiotrace in their kernel
> > > configurations by default, if it eats lots of memory for nothing?
> >
> > Hmm, good point. We could change the allocation to when it is first
> > enabled. Something other than 'nop' being put into the current_tracer
> > file.
>
> I'm very much looking forward to this.

Unfortunately, this is not a trivial change. I'll have to look into it.

>
> > >
> > > If distributors do not enable mmiotrace by default, we are in a worse
> > > situation than with out-of-tree mmiotrace module (if it could work).
> > > Users need to reconfigure and recompile their kernels, which is
> > > something we wanted to avoid. This is the reality right now.
> > >
> > > 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.
>
> The "nop" having no buffers at boot would be enough, but this would be
> even better.

We would need something separate from nop, since nop can still record. If
there is no buffers, its best not to record. Although, the ring buffers
should just return NULL if the buffer is not allocated yet (I better make
sure that is the case).

But just for accounting reasons, its best to have a "none" that does no
tracing what-so-ever. This way, you can echo 'none' back into the current
tracer, and have the buffers freed (?).

>
> > >
> > > > Now we have recently added /debug/tracing/tracing_on which can quickly
> > > > stop tracing. I may be able to use that, and we can let the
> > > > tracing_enable" reset it too.
> > >
> > > Does this mean I have to implement yet another on/off hook?
> >
> > Nope, the on/off hook is extremely fast, and the plugins do not even know
> > when they happen. The on/off simply turns off writing to the ring buffer.
> > The plugin functions will still be called, it is just that they will fail
> > to write to the ring buffers. As stated above, the
> > ring_buffer_reserve_lock will return a NULL.
>
> Does this also increment the lost events counters?

Unfortunately no. The lost events are about overruns. But the calling
tracer should easily be able to add these as well. They see the "NULL"
returned from the function. This means it obviously did not record the
event. The overruns, on the otherhand, is not visible to the tracer, so
the ring buffer keeps count.

>
> > > IMHO it is starting to be confusing having all these
> > > current_tracer, tracing_enabled, tracing_on etc.
>
> >
> > The tracing_enabled is the way to start and stop a trace. I'm considering
> > to implement Frederic's request to reset the buffer on enabled. This is
> > quick but requires locks and mutexes to be taken. It calls a hook to the
> > plugin because different plugins actually want to reset (the irq latency
> > tracers reset with this).
> >
> > The tracing_on is something that has been asked by developers to give a
> > way to start and stop tracing fast as possible, with no mutexes or added
> > locks. In fact, this option is local to the ring buffer code. Ftrace does
> > not even use it directly. It just a global flag to stop tracing. There's
> > also an in-kernel equivalent tracing_on() and tracing_off(). This just
> > sets or clears a global flag that will stop any more writes to the trace
> > buffer.
>
> Why not call tracing_on, say, logging_enabled?
> IMHO tracing_enabled vs. tracing_on is incomprehesible, but
> tracing_enabled vs. logging_enabled is a little more understandable.
> current_tracer is self-explanatory, and tracing_enabled used to be.

One of the hardest things about ftrace is coming up with good naming. This
is not always so easy, as you can tell. I'm not sure "logging_enabled" is
any better. That would confuse myself ;-) The reason I chose "on" is
because it is like an on/off switch, where as enabled/disabled is usually
(in the kernel anyway) associated to nesting.

How about tracing_enabled_light? ;-)

-- Steve

2008-12-20 02:56:55

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Sat, 20 Dec 2008, Pekka Paalanen wrote:

> >
> > To implement this at the ftrace level should be a trivial change. I'm just
> > saying that doing this at the "ring buffer" level might be a bit more
> > complex. The ring buffer has no idea of ftrace. It should not. It is at
> > a lower lever than ftrace. Although, I do think some of the protecting
> > that is done at the tracing level during resize should be moved down into
> > the ring buffer layer.
>
> Aah, so you are saying that the buffer_size file (or whatever it was called)
> is part of the ring buffer user API, and not tracing user API?

Nope, the buffer_size is part of the ftrace API. It was just that it
seemed that Ingo was pushing that the ring buffer API should handle it. I
may have misunderstood Ingo though. Note, when Ingo and I start going back
and forth, we sometimes are at the implementation level, and probably will
confuse the users ;-)

Since the buffer_size is at the ftrace level, it will make it easier to do
the changes there.

>
> But the ring buffer is just a buffer, is it meaningful to adjust a ring
> buffer size? I cannot tell tracing to go use a different buffer. And if
> there will be other users of ring buffers, they would probably want to
> have their own control over the buffer size.

Exactly.

>
> As a user, I want to adjust *the* tracing ring buffer size, not some ring
> buffer size.

Correct, and that is what you are doing.

>
> Am I making any sense? I'm trying to say that in my opinion, the
> buffer_size file does not belong to the "ring buffer" level. The upper
> levels should decide whether and how it offers buffer resizing.

The "buffer_size" file is part of ftrace, not the ring buffer. You are
making perfect sense.

-- Steve

2008-12-20 13:15:24

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper

2008/12/19 Steven Rostedt <[email protected]>:
>
> On Fri, 19 Dec 2008, Fr?d?ric Weisbecker wrote:
>>
>>
>> That's looks good.
>> 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.
>
> Now we have recently added /debug/tracing/tracing_on which can quickly
> stop tracing. I may be able to use that, and we can let the
> tracing_enable" reset it too.
>
> I'll have to take a look at my scripts to see if that would work.

Ok.
Actually perhaps that could be useful to do it only before calling the
init callback of a tracer.
That should be the only place where a tracer would want to reset the buffers....

2008-12-20 14:15:36

by Frederic Weisbecker

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

2008/12/20 Steven Rostedt <[email protected]>:
> 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 <[email protected]> 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.

2008-12-23 17:29:53

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Sat, 20 Dec 2008, Pekka Paalanen wrote:
>
> I thought this was just about not having to do
>
> $ echo 0 > tracing_enabled
> $ echo 28764243 > buffer_size
> $ echo 1 > tracing_enabled
>
> and instead just do
>
> $ echo 28764243 > buffer_size
>
> which would do exactly the same, except being easier for the user.
> Personally I've never dreamed of any kind of resize-in-flight.
>

I must be losing it :-p

You can on tip simply echo 28764243 > buffer_size without disabling the
tracer. This change is not in mainline, but I expect it will be in
2.6.29.

I just resized the buffer while running the function tracer. I made the
change a while ago.

-- Steve

2008-12-31 13:53:44

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Fri, 19 Dec 2008 21:47:53 -0500 (EST)
Steven Rostedt <[email protected]> wrote:

> > > The tracing_enabled is the way to start and stop a trace. I'm considering
> > > to implement Frederic's request to reset the buffer on enabled. This is
> > > quick but requires locks and mutexes to be taken. It calls a hook to the
> > > plugin because different plugins actually want to reset (the irq latency
> > > tracers reset with this).
> > >
> > > The tracing_on is something that has been asked by developers to give a
> > > way to start and stop tracing fast as possible, with no mutexes or added
> > > locks. In fact, this option is local to the ring buffer code. Ftrace does
> > > not even use it directly. It just a global flag to stop tracing. There's
> > > also an in-kernel equivalent tracing_on() and tracing_off(). This just
> > > sets or clears a global flag that will stop any more writes to the trace
> > > buffer.
> >
> > Why not call tracing_on, say, logging_enabled?
> > IMHO tracing_enabled vs. tracing_on is incomprehesible, but
> > tracing_enabled vs. logging_enabled is a little more understandable.
> > current_tracer is self-explanatory, and tracing_enabled used to be.
>
> One of the hardest things about ftrace is coming up with good naming. This
> is not always so easy, as you can tell. I'm not sure "logging_enabled" is
> any better. That would confuse myself ;-) The reason I chose "on" is
> because it is like an on/off switch, where as enabled/disabled is usually
> (in the kernel anyway) associated to nesting.
>
> How about tracing_enabled_light? ;-)

"tracing_on" is not a function call, so nesting does not make sense.
It is a file in debugfs, and therefore an end user interface. I am only
talking about the debugfs files, not functions. Only developers use
functions and devels can be expected to find the implementation and
read the fine documentation.

How about /debug/tracing/recording with values 0 and 1?
That would be a clear distinction to /debug/tracing/tracing_enabled.
I have also wondered about the "tracing_" prefix, the files are
already in the tracing/ directory.

I am just trying to think what file names would make sense in
debugfs. I completely agree with you on function naming, but should
the file have the same name as the function? Sure, for developers
it would be easier to remember, but the name might not make any
sense unless you know the internal implementation.

If something else than tracing starts to use the ring buffer
facility, we have to think about the names again. Until then,
just my 2c. :-)

Thanks.

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-31 18:57:25

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Wed, 31 Dec 2008 13:33:19 -0500 (EST)
Steven Rostedt <[email protected]> wrote:

> On Wed, 31 Dec 2008, Pekka Paalanen wrote:
> >
> > "tracing_on" is not a function call, so nesting does not make sense.
> > It is a file in debugfs, and therefore an end user interface. I am only
> > talking about the debugfs files, not functions. Only developers use
> > functions and devels can be expected to find the implementation and
> > read the fine documentation.
> >
> > How about /debug/tracing/recording with values 0 and 1?
> > That would be a clear distinction to /debug/tracing/tracing_enabled.
> > I have also wondered about the "tracing_" prefix, the files are
> > already in the tracing/ directory.
> >
> > I am just trying to think what file names would make sense in
> > debugfs. I completely agree with you on function naming, but should
> > the file have the same name as the function? Sure, for developers
> > it would be easier to remember, but the name might not make any
> > sense unless you know the internal implementation.
> >
> > If something else than tracing starts to use the ring buffer
> > facility, we have to think about the names again. Until then,
> > just my 2c. :-)
>
> Since this really only enables or disables the ring buffer, perhaps
> "ringbuffer_enabled" is the way to go?

As a C-function or as a debugfs file?
Are we controlling an action (recording events), a feature (a buffer
where to record) or an implementation (a ring buffer)?
Does the user know or care if it is a ring buffer or just whatever
temporary storage?
What does the user actually want to control? A buffer? A ring
buffer? Recording stuff? The tracer? Tracing? Data flow?
Assuming there are also other users than tracing, does it make
sense to control the ring buffer facility itself?

I'm sure we could discuss this forever, so I'll just say:
Happy new year! :-)

--
Pekka Paalanen
http://www.iki.fi/pq/

2008-12-31 18:33:32

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Wed, 31 Dec 2008, Pekka Paalanen wrote:
>
> "tracing_on" is not a function call, so nesting does not make sense.
> It is a file in debugfs, and therefore an end user interface. I am only
> talking about the debugfs files, not functions. Only developers use
> functions and devels can be expected to find the implementation and
> read the fine documentation.
>
> How about /debug/tracing/recording with values 0 and 1?
> That would be a clear distinction to /debug/tracing/tracing_enabled.
> I have also wondered about the "tracing_" prefix, the files are
> already in the tracing/ directory.
>
> I am just trying to think what file names would make sense in
> debugfs. I completely agree with you on function naming, but should
> the file have the same name as the function? Sure, for developers
> it would be easier to remember, but the name might not make any
> sense unless you know the internal implementation.
>
> If something else than tracing starts to use the ring buffer
> facility, we have to think about the names again. Until then,
> just my 2c. :-)

Since this really only enables or disables the ring buffer, perhaps
"ringbuffer_enabled" is the way to go?

-- Steve

2008-12-31 19:06:37

[permalink] [raw]

Subject: Re: ftrace behaviour (was: [PATCH] ftrace: introduce tracing_reset_online_cpus() helper)

On Wed, 31 Dec 2008, Pekka Paalanen wrote:

> >
> > Since this really only enables or disables the ring buffer, perhaps
> > "ringbuffer_enabled" is the way to go?
>
> As a C-function or as a debugfs file?

I was thinking of only changing the debugfs file.

> Are we controlling an action (recording events), a feature (a buffer
> where to record) or an implementation (a ring buffer)?

Good point. It only disables the recording, so perhaps a "record_enabled"
would be better?

> Does the user know or care if it is a ring buffer or just whatever
> temporary storage?

Yeah, the user does not know or care what the implementation is.

> What does the user actually want to control? A buffer? A ring
> buffer? Recording stuff? The tracer? Tracing? Data flow?
> Assuming there are also other users than tracing, does it make
> sense to control the ring buffer facility itself?

I think the name record_enabled for debugfs is the best. This is exactly
what happens (not how it is implemented). When someone echos 0 to
record_enabled (currently called tracing_on), it stops the recording, and
nothing else. The tracers still try to write to the buffer, but the write
always fails. This does not disable the tracers or even notify the tracer
that the buffers have stopped recording. This is just a simple light
weight way to stop and start recording to the trace buffers from either
user space or kernel space. Kernel space can stop it, and user space can
start it again (that was the original request for this feature).

I'm leaning towards record_enabled now.

>
> I'm sure we could discuss this forever, so I'll just say:
> Happy new year! :-)

I still have 10 more hours, but who cares? ;-)

Happy New Year to you too!

-- Steve

2008-12-31 22:08:28