[PATCH] ftrace: introduce tracing_reset_online_cpus() helper

[PATCH] ftrace: introduce tracing_reset_online_cpus() helper

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

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 <penberg@cs.helsinki.fi>
---
 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(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1a3d6b3..c39a0d8 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -679,6 +679,16 @@ void tracing_reset(struct trace_array *tr, int cpu)
 	ftrace_enable_cpu();
 }
 
+void tracing_reset_online_cpus(struct trace_array *tr)
+{
+	int cpu;
+
+	tr->time_start = ftrace_now(tr->cpu);
+
+	for_each_online_cpu(cpu)
+		tracing_reset(tr, cpu);
+}
+
 #define SAVED_CMDLINES 128
 static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
 static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index fc75dce..cc7a4f8 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -374,6 +374,7 @@ struct trace_iterator {
 int tracing_is_enabled(void);
 void trace_wake_up(void);
 void tracing_reset(struct trace_array *tr, int cpu);
+void tracing_reset_online_cpus(struct trace_array *tr);
 int tracing_open_generic(struct inode *inode, struct file *filp);
 struct dentry *tracing_init_dentry(void);
 void init_tracer_sysprof_debugfs(struct dentry *d_tracer);
diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index a4fa2c5..3ccebde 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -37,16 +37,6 @@ void disable_boot_trace(void)
 		tracing_stop_sched_switch_record();
 }
 
-static void reset_boot_trace(struct trace_array *tr)
-{
-	int cpu;
-
-	tr->time_start = ftrace_now(tr->cpu);
-
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-}
-
 static int boot_trace_init(struct trace_array *tr)
 {
 	int cpu;
@@ -130,7 +120,7 @@ struct tracer boot_tracer __read_mostly =
 {
 	.name		= "initcall",
 	.init		= boot_trace_init,
-	.reset		= reset_boot_trace,
+	.reset		= tracing_reset_online_cpus,
 	.print_line	= initcall_print_line,
 };
 
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index e74f6d0..9236d7e 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -16,20 +16,10 @@
 
 #include "trace.h"
 
-static void function_reset(struct trace_array *tr)
-{
-	int cpu;
-
-	tr->time_start = ftrace_now(tr->cpu);
-
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-}
-
 static void start_function_trace(struct trace_array *tr)
 {
 	tr->cpu = get_cpu();
-	function_reset(tr);
+	tracing_reset_online_cpus(tr);
 	put_cpu();
 
 	tracing_start_cmdline_record();
@@ -55,7 +45,7 @@ static void function_trace_reset(struct trace_array *tr)
 
 static void function_trace_start(struct trace_array *tr)
 {
-	function_reset(tr);
+	tracing_reset_online_cpus(tr);
 }
 
 static struct tracer function_trace __read_mostly =
diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c
index ee29e01..b6a3e20 100644
--- a/kernel/trace/trace_hw_branches.c
+++ b/kernel/trace/trace_hw_branches.c
@@ -25,16 +25,6 @@ static DEFINE_PER_CPU(unsigned char[SIZEOF_BTS], buffer);
 #define this_buffer per_cpu(buffer, smp_processor_id())
 
 
-static void bts_trace_reset(struct trace_array *tr)
-{
-	int cpu;
-
-	tr->time_start = ftrace_now(tr->cpu);
-
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-}
-
 static void bts_trace_start_cpu(void *arg)
 {
 	if (this_tracer)
@@ -54,7 +44,7 @@ static void bts_trace_start(struct trace_array *tr)
 {
 	int cpu;
 
-	bts_trace_reset(tr);
+	tracing_reset_online_cpus(tr);
 
 	for_each_cpu_mask(cpu, cpu_possible_map)
 		smp_call_function_single(cpu, bts_trace_start_cpu, NULL, 1);
@@ -78,7 +68,7 @@ static void bts_trace_stop(struct trace_array *tr)
 
 static int bts_trace_init(struct trace_array *tr)
 {
-	bts_trace_reset(tr);
+	tracing_reset_online_cpus(tr);
 	bts_trace_start(tr);
 
 	return 0;
diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index 2fb6da6..fffcb06 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -22,14 +22,10 @@ static unsigned long prev_overruns;
 
 static void mmio_reset_data(struct trace_array *tr)
 {
-	int cpu;
-
 	overrun_detected = false;
 	prev_overruns = 0;
-	tr->time_start = ftrace_now(tr->cpu);
 
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
+	tracing_reset_online_cpus(tr);
 }
 
 static int mmio_trace_init(struct trace_array *tr)
diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
index 8633905..7f59f91 100644
--- a/kernel/trace/trace_sched_switch.c
+++ b/kernel/trace/trace_sched_switch.c
@@ -72,16 +72,6 @@ probe_sched_wakeup(struct rq *__rq, struct task_struct *wakee)
 	local_irq_restore(flags);
 }
 
-static void sched_switch_reset(struct trace_array *tr)
-{
-	int cpu;
-
-	tr->time_start = ftrace_now(tr->cpu);
-
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-}
-
 static int tracing_sched_register(void)
 {
 	int ret;
@@ -197,7 +187,7 @@ void tracing_sched_switch_assign_trace(struct trace_array *tr)
 
 static void start_sched_trace(struct trace_array *tr)
 {
-	sched_switch_reset(tr);
+	tracing_reset_online_cpus(tr);
 	tracing_start_sched_switch_record();
 }
 
@@ -221,7 +211,7 @@ static void sched_switch_trace_reset(struct trace_array *tr)
 
 static void sched_switch_trace_start(struct trace_array *tr)
 {
-	sched_switch_reset(tr);
+	tracing_reset_online_cpus(tr);
 	tracing_start_sched_switch();
 }
 
diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
index 54960ed..01becf1 100644
--- a/kernel/trace/trace_sysprof.c
+++ b/kernel/trace/trace_sysprof.c
@@ -234,20 +234,10 @@ static void stop_stack_timers(void)
 		stop_stack_timer(cpu);
 }
 
-static void stack_reset(struct trace_array *tr)
-{
-	int cpu;
-
-	tr->time_start = ftrace_now(tr->cpu);
-
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-}
-
 static void start_stack_trace(struct trace_array *tr)
 {
 	mutex_lock(&sample_timer_lock);
-	stack_reset(tr);
+	tracing_reset_online_cpus(tr);
 	start_stack_timers();
 	tracer_enabled = 1;
 	mutex_unlock(&sample_timer_lock);
-- 
1.5.4.3

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

[Frédéric Weisbecker]

2008/12/19 Pekka J Enberg <penberg@cs.helsinki.fi>:
> From: Pekka Enberg <penberg@cs.helsinki.fi>
>
> 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 <penberg@cs.helsinki.fi>
> ---
>  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(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 1a3d6b3..c39a0d8 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -679,6 +679,16 @@ void tracing_reset(struct trace_array *tr, int cpu)
>        ftrace_enable_cpu();
>  }
>
> +void tracing_reset_online_cpus(struct trace_array *tr)
> +{
> +       int cpu;
> +
> +       tr->time_start = ftrace_now(tr->cpu);
> +
> +       for_each_online_cpu(cpu)
> +               tracing_reset(tr, cpu);
> +}
> +
>  #define SAVED_CMDLINES 128
>  static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
>  static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index fc75dce..cc7a4f8 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -374,6 +374,7 @@ struct trace_iterator {
>  int tracing_is_enabled(void);
>  void trace_wake_up(void);
>  void tracing_reset(struct trace_array *tr, int cpu);
> +void tracing_reset_online_cpus(struct trace_array *tr);
>  int tracing_open_generic(struct inode *inode, struct file *filp);
>  struct dentry *tracing_init_dentry(void);
>  void init_tracer_sysprof_debugfs(struct dentry *d_tracer);
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index a4fa2c5..3ccebde 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -37,16 +37,6 @@ void disable_boot_trace(void)
>                tracing_stop_sched_switch_record();
>  }
>
> -static void reset_boot_trace(struct trace_array *tr)
> -{
> -       int cpu;
> -
> -       tr->time_start = ftrace_now(tr->cpu);
> -
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> -}
> -
>  static int boot_trace_init(struct trace_array *tr)
>  {
>        int cpu;
> @@ -130,7 +120,7 @@ struct tracer boot_tracer __read_mostly =
>  {
>        .name           = "initcall",
>        .init           = boot_trace_init,
> -       .reset          = reset_boot_trace,
> +       .reset          = tracing_reset_online_cpus,
>        .print_line     = initcall_print_line,
>  };
>
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index e74f6d0..9236d7e 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -16,20 +16,10 @@
>
>  #include "trace.h"
>
> -static void function_reset(struct trace_array *tr)
> -{
> -       int cpu;
> -
> -       tr->time_start = ftrace_now(tr->cpu);
> -
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> -}
> -
>  static void start_function_trace(struct trace_array *tr)
>  {
>        tr->cpu = get_cpu();
> -       function_reset(tr);
> +       tracing_reset_online_cpus(tr);
>        put_cpu();
>
>        tracing_start_cmdline_record();
> @@ -55,7 +45,7 @@ static void function_trace_reset(struct trace_array *tr)
>
>  static void function_trace_start(struct trace_array *tr)
>  {
> -       function_reset(tr);
> +       tracing_reset_online_cpus(tr);
>  }
>
>  static struct tracer function_trace __read_mostly =
> diff --git a/kernel/trace/trace_hw_branches.c b/kernel/trace/trace_hw_branches.c
> index ee29e01..b6a3e20 100644
> --- a/kernel/trace/trace_hw_branches.c
> +++ b/kernel/trace/trace_hw_branches.c
> @@ -25,16 +25,6 @@ static DEFINE_PER_CPU(unsigned char[SIZEOF_BTS], buffer);
>  #define this_buffer per_cpu(buffer, smp_processor_id())
>
>
> -static void bts_trace_reset(struct trace_array *tr)
> -{
> -       int cpu;
> -
> -       tr->time_start = ftrace_now(tr->cpu);
> -
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> -}
> -
>  static void bts_trace_start_cpu(void *arg)
>  {
>        if (this_tracer)
> @@ -54,7 +44,7 @@ static void bts_trace_start(struct trace_array *tr)
>  {
>        int cpu;
>
> -       bts_trace_reset(tr);
> +       tracing_reset_online_cpus(tr);
>
>        for_each_cpu_mask(cpu, cpu_possible_map)
>                smp_call_function_single(cpu, bts_trace_start_cpu, NULL, 1);
> @@ -78,7 +68,7 @@ static void bts_trace_stop(struct trace_array *tr)
>
>  static int bts_trace_init(struct trace_array *tr)
>  {
> -       bts_trace_reset(tr);
> +       tracing_reset_online_cpus(tr);
>        bts_trace_start(tr);
>
>        return 0;
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index 2fb6da6..fffcb06 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -22,14 +22,10 @@ static unsigned long prev_overruns;
>
>  static void mmio_reset_data(struct trace_array *tr)
>  {
> -       int cpu;
> -
>        overrun_detected = false;
>        prev_overruns = 0;
> -       tr->time_start = ftrace_now(tr->cpu);
>
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> +       tracing_reset_online_cpus(tr);
>  }
>
>  static int mmio_trace_init(struct trace_array *tr)
> diff --git a/kernel/trace/trace_sched_switch.c b/kernel/trace/trace_sched_switch.c
> index 8633905..7f59f91 100644
> --- a/kernel/trace/trace_sched_switch.c
> +++ b/kernel/trace/trace_sched_switch.c
> @@ -72,16 +72,6 @@ probe_sched_wakeup(struct rq *__rq, struct task_struct *wakee)
>        local_irq_restore(flags);
>  }
>
> -static void sched_switch_reset(struct trace_array *tr)
> -{
> -       int cpu;
> -
> -       tr->time_start = ftrace_now(tr->cpu);
> -
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> -}
> -
>  static int tracing_sched_register(void)
>  {
>        int ret;
> @@ -197,7 +187,7 @@ void tracing_sched_switch_assign_trace(struct trace_array *tr)
>
>  static void start_sched_trace(struct trace_array *tr)
>  {
> -       sched_switch_reset(tr);
> +       tracing_reset_online_cpus(tr);
>        tracing_start_sched_switch_record();
>  }
>
> @@ -221,7 +211,7 @@ static void sched_switch_trace_reset(struct trace_array *tr)
>
>  static void sched_switch_trace_start(struct trace_array *tr)
>  {
> -       sched_switch_reset(tr);
> +       tracing_reset_online_cpus(tr);
>        tracing_start_sched_switch();
>  }
>
> diff --git a/kernel/trace/trace_sysprof.c b/kernel/trace/trace_sysprof.c
> index 54960ed..01becf1 100644
> --- a/kernel/trace/trace_sysprof.c
> +++ b/kernel/trace/trace_sysprof.c
> @@ -234,20 +234,10 @@ static void stop_stack_timers(void)
>                stop_stack_timer(cpu);
>  }
>
> -static void stack_reset(struct trace_array *tr)
> -{
> -       int cpu;
> -
> -       tr->time_start = ftrace_now(tr->cpu);
> -
> -       for_each_online_cpu(cpu)
> -               tracing_reset(tr, cpu);
> -}
> -
>  static void start_stack_trace(struct trace_array *tr)
>  {
>        mutex_lock(&sample_timer_lock);
> -       stack_reset(tr);
> +       tracing_reset_online_cpus(tr);
>        start_stack_timers();
>        tracer_enabled = 1;
>        mutex_unlock(&sample_timer_lock);


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.

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

[Ingo Molnar]

* Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> 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 <penberg@cs.helsinki.fi>
> ---
>  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

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

[Steven Rostedt]

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

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

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> 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

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

[Pekka Paalanen]

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

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/

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

[Ingo Molnar]

* Pekka Paalanen <pq@iki.fi> 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

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

[Steven Rostedt]

On Fri, 19 Dec 2008, Ingo Molnar wrote:

> 
> * Pekka Paalanen <pq@iki.fi> 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

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

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> 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

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

[Steven Rostedt]

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.

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

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

[Steven Rostedt]

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

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

[Pekka Paalanen]

On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> 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/

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

[Steven Rostedt]

On Sat, 20 Dec 2008, Pekka Paalanen wrote:

> On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
> Steven Rostedt <rostedt@goodmis.org> 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

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

[Pekka Paalanen]

Steven,

thank you for a complete reply. Few comments below.

On Fri, 19 Dec 2008 19:03:42 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> 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/

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

[Pekka Paalanen]

On Fri, 19 Dec 2008 20:46:38 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Sat, 20 Dec 2008, Pekka Paalanen wrote:
> 
> > On Fri, 19 Dec 2008 19:29:30 -0500 (EST)
> > Steven Rostedt <rostedt@goodmis.org> 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/

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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

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

[Frédéric Weisbecker]

2008/12/19 Steven Rostedt <rostedt@goodmis.org>:
>
> 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....

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

[Frédéric Weisbecker]

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.

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

[Steven Rostedt]

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

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

[Pekka Paalanen]

On Fri, 19 Dec 2008 21:47:53 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> 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/

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

[Steven Rostedt]

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

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

[Pekka Paalanen]

On Wed, 31 Dec 2008 13:33:19 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> 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/

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

[Steven Rostedt]

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

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

[Pekka Paalanen]

On Wed, 31 Dec 2008 14:06:26 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> wrote:

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

To me "record" sounds more of a noun than a verb, but it's both and
I'm not a native speaker. Still, it brings me to "recording_enabled",
and do we really need the "_enabled" part? So we end up to what I
suggested earlier: "recording" with values 0 and 1. :-)

Anyway, it's good to start the file name with a few distinct letters,
it makes tab-completion so much easier on the command line.

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

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