[PATCH 0/2] [GIT PULL][3.9] tracing: Fix in snapshot API

[PATCH 0/2] [GIT PULL][3.9] tracing: Fix in snapshot API

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]


Ingo,

The snapshot feature has been added in the 3.9 merge window. But after
playing with it some more, I've come to realize that it needs a
self documenting help (I had to look at the code a few times to remember
how to use it, or look in Documentation), and it also gives an awkward
error code if you go to clear the buffer and the buffer does not exist.

These two patches fix this issues, and I strongly suggest they go in
before snapshot is part of a major release.

The first patch gives the user a nice description to how to use snapshot:

     # tracer: nop
     #
     #
     # * Snapshot is freed *
     #
     # Snapshot commands:
     # echo 0 > snapshot : Clears and frees snapshot buffer
     # echo 1 > snapshot : Allocates snapshot buffer, if not already allocated.
     #                      Takes a snapshot of the main buffer.
     # echo 2 > snapshot : Clears snapshot buffer (but does not allocate)
     #                      (Doesn't have to be '2' works with any number that
     #                       is not a '0' or '1')

as well as the state of the snapshot buffer (freed or allocated).

The second patch, returns success on a reset of the buffer even if
the buffer wasn't allocated. Returning -EINVAL is just confusing.

These should go into mainline before 3.9 is released.

Thanks!

Please pull the latest tip/perf/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent

Head SHA1: c9960e48543799f168c4c9486f9790fb686ce5a8


Steven Rostedt (Red Hat) (2):
      tracing: Add help of snapshot feature when snapshot is empty
      tracing: Do not return EINVAL in snapshot when not allocated

----
 kernel/trace/trace.c |   27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH 1/2] tracing: Add help of snapshot feature when snapshot is empty

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 3524 bytes --]

From: "Steven Rostedt (Red Hat)" <srostedt@redhat.com>

When cat'ing the snapshot file, instead of showing an empty trace
header like the trace file does, show how to use the snapshot
feature.

Also, this is a good place to show if the snapshot has been allocated
or not. Users may want to "pre allocate" the snapshot to have a fast
"swap" of the current buffer. Otherwise, a swap would be slow and might
fail as it would need to allocate the snapshot buffer, and that might
fail under tight memory constraints.

Here's what it looked like before:

 # tracer: nop
 #
 # entries-in-buffer/entries-written: 0/0   #P:4
 #
 #                              _-----=> irqs-off
 #                             / _----=> need-resched
 #                            | / _---=> hardirq/softirq
 #                            || / _--=> preempt-depth
 #                            ||| /     delay
 #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
 #              | |       |   ||||       |         |

Here's what it looks like now:

 # tracer: nop
 #
 #
 # * Snapshot is freed *
 #
 # Snapshot commands:
 # echo 0 > snapshot : Clears and frees snapshot buffer
 # echo 1 > snapshot : Allocates snapshot buffer, if not already allocated.
 #                      Takes a snapshot of the main buffer.
 # echo 2 > snapshot : Clears snapshot buffer (but does not allocate)
 #                      (Doesn't have to be '2' works with any number that
 #                       is not a '0' or '1')

Acked-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c2e2c23..9e3120b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2400,6 +2400,27 @@ static void test_ftrace_alive(struct seq_file *m)
 	seq_printf(m, "#          MAY BE MISSING FUNCTION EVENTS\n");
 }
 
+#ifdef CONFIG_TRACER_MAX_TRACE
+static void print_snapshot_help(struct seq_file *m, struct trace_iterator *iter)
+{
+	if (iter->trace->allocated_snapshot)
+		seq_printf(m, "#\n# * Snapshot is allocated *\n#\n");
+	else
+		seq_printf(m, "#\n# * Snapshot is freed *\n#\n");
+
+	seq_printf(m, "# Snapshot commands:\n");
+	seq_printf(m, "# echo 0 > snapshot : Clears and frees snapshot buffer\n");
+	seq_printf(m, "# echo 1 > snapshot : Allocates snapshot buffer, if not already allocated.\n");
+	seq_printf(m, "#                      Takes a snapshot of the main buffer.\n");
+	seq_printf(m, "# echo 2 > snapshot : Clears snapshot buffer (but does not allocate)\n");
+	seq_printf(m, "#                      (Doesn't have to be '2' works with any number that\n");
+	seq_printf(m, "#                       is not a '0' or '1')\n");
+}
+#else
+/* Should never be called */
+static inline void print_snapshot_help(struct seq_file *m, struct trace_iterator *iter) { }
+#endif
+
 static int s_show(struct seq_file *m, void *v)
 {
 	struct trace_iterator *iter = v;
@@ -2411,7 +2432,9 @@ static int s_show(struct seq_file *m, void *v)
 			seq_puts(m, "#\n");
 			test_ftrace_alive(m);
 		}
-		if (iter->trace && iter->trace->print_header)
+		if (iter->snapshot && trace_empty(iter))
+			print_snapshot_help(m, iter);
+		else if (iter->trace && iter->trace->print_header)
 			iter->trace->print_header(m);
 		else
 			trace_default_header(m);
-- 
1.7.10.4



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH 2/2] tracing: Do not return EINVAL in snapshot when not allocated

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

From: "Steven Rostedt (Red Hat)" <srostedt@redhat.com>

To use the tracing snapshot feature, writing a '1' into the snapshot
file causes the snapshot buffer to be allocated if it has not already
been allocated and dose a 'swap' with the main buffer, so that the
snapshot now contains what was in the main buffer, and the main buffer
now writes to what was the snapshot buffer.

To free the snapshot buffer, a '0' is written into the snapshot file.

To clear the snapshot buffer, any number but a '0' or '1' is written
into the snapshot file. But if the file is not allocated it returns
-EINVAL error code. This is rather pointless. It is better just to
do nothing and return success.

Acked-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 9e3120b..1f835a8 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4167,8 +4167,6 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	default:
 		if (current_trace->allocated_snapshot)
 			tracing_reset_online_cpus(&max_tr);
-		else
-			ret = -EINVAL;
 		break;
 	}
 
-- 
1.7.10.4



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 0/2] [GIT PULL][3.9] tracing: Fix in snapshot API

[Hiraku Toyooka]

Steven,

(03/08/2013 12:34 AM), Steven Rostedt wrote:
> The second patch, returns success on a reset of the buffer even if
> the buffer wasn't allocated. Returning -EINVAL is just confusing.

I realized we should update the snapshot documentation together with
this change.
I attached a patch to update the documentation. Could you include this
patch?

Thanks,
Hiraku Toyooka


From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Subject: [PATCH] tracing: update documentation of snapshot utility

Now, "snapshot" file returns success on a reset of snapshot buffer
even if the buffer wasn't allocated, instead of returning EINVAL.
This patch updates snapshot desctiption according to the change.

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
---
  Documentation/trace/ftrace.txt | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index 53d6a3c..a372304 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -1873,7 +1873,7 @@ feature:

  	status\input  |     0      |     1      |    else    |
  	--------------+------------+------------+------------+
-	not allocated |(do nothing)| alloc+swap |   EINVAL   |
+	not allocated |(do nothing)| alloc+swap |(do nothing)|
  	--------------+------------+------------+------------+
  	allocated     |    free    |    swap    |   clear    |
  	--------------+------------+------------+------------+
-- 
1.7.11.7

Re: [PATCH 0/2] [GIT PULL][3.9] tracing: Fix in snapshot API

[Ingo Molnar]

* Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com> wrote:

> Steven,
> 
> (03/08/2013 12:34 AM), Steven Rostedt wrote:
> >The second patch, returns success on a reset of the buffer even if
> >the buffer wasn't allocated. Returning -EINVAL is just confusing.
> 
> I realized we should update the snapshot documentation together with
> this change.
> I attached a patch to update the documentation. Could you include this
> patch?
> 
> Thanks,
> Hiraku Toyooka
> 
> 
> From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> Subject: [PATCH] tracing: update documentation of snapshot utility
> 
> Now, "snapshot" file returns success on a reset of snapshot buffer
> even if the buffer wasn't allocated, instead of returning EINVAL.
> This patch updates snapshot desctiption according to the change.
> 
> Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
> ---
>  Documentation/trace/ftrace.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
> index 53d6a3c..a372304 100644
> --- a/Documentation/trace/ftrace.txt
> +++ b/Documentation/trace/ftrace.txt
> @@ -1873,7 +1873,7 @@ feature:
> 
>  	status\input  |     0      |     1      |    else    |
>  	--------------+------------+------------+------------+
> -	not allocated |(do nothing)| alloc+swap |   EINVAL   |
> +	not allocated |(do nothing)| alloc+swap |(do nothing)|
>  	--------------+------------+------------+------------+
>  	allocated     |    free    |    swap    |   clear    |
>  	--------------+------------+------------+------------+

I pulled all the updates - thanks guys!

	Ingo

[tip:perf/urgent] tracing: update documentation of snapshot utility

[tip-bot for Hiraku Toyooka]

Commit-ID:  1abccd7419de9829bcdf9ab1f81d5f6cf74d55d3
Gitweb:     http://git.kernel.org/tip/1abccd7419de9829bcdf9ab1f81d5f6cf74d55d3
Author:     Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
AuthorDate: Fri, 8 Mar 2013 16:32:25 +0900
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 8 Mar 2013 06:27:11 -0500

tracing: update documentation of snapshot utility

Now, "snapshot" file returns success on a reset of snapshot buffer
even if the buffer wasn't allocated, instead of returning EINVAL.
This patch updates snapshot desctiption according to the change.

Link: http://lkml.kernel.org/r/51399409.4090207@hitachi.com

Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/trace/ftrace.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index 53d6a3c..a372304 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -1873,7 +1873,7 @@ feature:
 
 	status\input  |     0      |     1      |    else    |
 	--------------+------------+------------+------------+
-	not allocated |(do nothing)| alloc+swap |   EINVAL   |
+	not allocated |(do nothing)| alloc+swap |(do nothing)|
 	--------------+------------+------------+------------+
 	allocated     |    free    |    swap    |   clear    |
 	--------------+------------+------------+------------+