[PATCH v2 0/4] ftrace: important updates

[PATCH v2 0/4] ftrace: important updates

[Steven Rostedt]

Ingo,

The first patch is a critical fix that needs to get into 2.6.29.

The next patch is a rename of tracing_on to writing_enabled since
tracing_on is confusing, and the file enables or disables writes.

The last patch is a fix to tip that prevents a memory leak.

This version I added some clean ups that Andrew Morton suggested.
I know you did not pull the previous version, because I forgot
to push the changes to kernel.org ;-)

The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/devel


Frederic Weisbecker (1):
      tracing/ftrace: fix a memory leak in stat tracing

Steven Rostedt (3):
      ftrace: convert unsigned index to signed
      ring-buffer: rename debugfs file tracing_on to writing_enabled
      trace: clean up funny line breaks in stat_seq_show

----
 kernel/trace/ftrace.c      |    4 ++--
 kernel/trace/ring_buffer.c |    5 +++--
 kernel/trace/trace_stat.c  |   40 ++++++++++++++++------------------------
 3 files changed, 21 insertions(+), 28 deletions(-)

-- 

[PATCH v2 1/4] ftrace: convert unsigned index to signed

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-convert-unsigned-index-to-signed.patch --]
[-- Type: text/plain, Size: 1352 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: fix to unsigned compared to less than zero

Roel Kluin pointed out that there is a compare of an unsigned number
to less than zero. A previous clean up had the unsigned index set
to -1 for certain cases, but never converted it to signed.

Frederic Weisbecker noticed that another index is used to compare
the above index to and it also needs to be converted to signed.

[
  Converted ftrace_page->index to int from unsigned long as
  Andrew Morton pointed out that there's no need for it to
  stay a long.
]

Reported-by: Roel Kluin <roel.kluin@gmail.com>
Reported-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 2f32969..9e54a6c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -289,7 +289,7 @@ static DEFINE_MUTEX(ftrace_regex_lock);
 
 struct ftrace_page {
 	struct ftrace_page	*next;
-	unsigned long		index;
+	int			index;
 	struct dyn_ftrace	records[];
 };
 
@@ -786,7 +786,7 @@ enum {
 
 struct ftrace_iterator {
 	struct ftrace_page	*pg;
-	unsigned		idx;
+	int			idx;
 	unsigned		flags;
 	unsigned char		buffer[FTRACE_BUFF_MAX+1];
 	unsigned		buffer_idx;
-- 
1.5.6.5

-- 

[PATCH v2 2/4] ring-buffer: rename debugfs file tracing_on to writing_enabled

[Steven Rostedt]

[-- Attachment #1: 0002-ring-buffer-rename-debugfs-file-tracing_on-to-writi.patch --]
[-- Type: text/plain, Size: 1891 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

The debugfs file name tracing_on is confusing since there is also
a tracing_enabled. When a tracer is added to current_tracer and
tracing_enabled is set to 1, then the tracer will start tracing.
The current file tracing_on, if set to 1, will allow writing to
the ring buffers. If tracing_on is set to 0 then the ring buffers
are read only, and any attempt by the tracers to write to the
ring buffers will fail.

But the name tracing_on confuses both developers and users.
This patch renames it to 'writing_enabled'.  This is exactly what
the file does. When set to 1, the ring buffers have writing enabled
and when set to 0, the ring buffers can not be written to.

The tracers do not get any call backs called or other notification
that the ring buffers have been set to read only, except when they
attempt to write to the ring buffers, that write will fail.

Note: it is a requirement that any tracer that uses the ring buffers
must handle a failure to write to the ring buffers, since the ring buffers
may be disabled for any number of reasons.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index a9d9760..dba6876 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2514,10 +2514,11 @@ static __init int rb_init_debugfs(void)
 
 	d_tracer = tracing_init_dentry();
 
-	entry = debugfs_create_file("tracing_on", 0644, d_tracer,
+	entry = debugfs_create_file("writing_enabled", 0644, d_tracer,
 				    &ring_buffer_flags, &rb_simple_fops);
 	if (!entry)
-		pr_warning("Could not create debugfs 'tracing_on' entry\n");
+		pr_warning("Could not create debugfs"
+			   " 'writing_enabled' entry\n");
 
 	return 0;
 }
-- 
1.5.6.5

-- 

[PATCH v2 3/4] tracing/ftrace: fix a memory leak in stat tracing

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-ftrace-fix-a-memory-leak-in-stat-tracing.patch --]
[-- Type: text/plain, Size: 4052 bytes --]

From: Frederic Weisbecker <fweisbec@gmail.com>

Impact: fix memory leak

This patch fixes a memory leak inside reset_stat_list(). The freeing
loop iterated only once.

Also turn the stat_list into a simple struct list_head, which
simplify the code and avoid an unused static pointer.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace_stat.c |   39 +++++++++++++++------------------------
 1 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index 6f194a3..4cb4ff2 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -21,7 +21,7 @@ struct trace_stat_list {
 	void *stat;
 };
 
-static struct trace_stat_list stat_list;
+static LIST_HEAD(stat_list);
 
 /*
  * This is a copy of the current tracer to avoid racy
@@ -39,22 +39,12 @@ static DEFINE_MUTEX(stat_list_mutex);
 
 static void reset_stat_list(void)
 {
-	struct trace_stat_list *node;
-	struct list_head *next;
+	struct trace_stat_list *node, *next;
 
-	if (list_empty(&stat_list.list))
-		return;
-
-	node = list_entry(stat_list.list.next, struct trace_stat_list, list);
-	next = node->list.next;
-
-	while (&node->list != next) {
+	list_for_each_entry_safe(node, next, &stat_list, list)
 		kfree(node);
-		node = list_entry(next, struct trace_stat_list, list);
-	}
-	kfree(node);
 
-	INIT_LIST_HEAD(&stat_list.list);
+	INIT_LIST_HEAD(&stat_list);
 }
 
 void init_tracer_stat(struct tracer *trace)
@@ -107,7 +97,7 @@ static int stat_seq_init(void)
 	}
 
 	INIT_LIST_HEAD(&new_entry->list);
-	list_add(&new_entry->list, &stat_list.list);
+	list_add(&new_entry->list, &stat_list);
 	new_entry->stat = current_tracer.stat_start();
 
 	prev_stat = new_entry->stat;
@@ -130,7 +120,7 @@ static int stat_seq_init(void)
 		if (!new_entry->stat)
 			break;
 
-		list_for_each_entry(iter_entry, &stat_list.list, list) {
+		list_for_each_entry(iter_entry, &stat_list, list) {
 			/* Insertion with a descendent sorting */
 			if (current_tracer.stat_cmp(new_entry->stat,
 						iter_entry->stat) > 0) {
@@ -141,7 +131,7 @@ static int stat_seq_init(void)
 
 			/* The current smaller value */
 			} else if (list_is_last(&iter_entry->list,
-						&stat_list.list)) {
+						&stat_list)) {
 				list_add(&new_entry->list, &iter_entry->list);
 				break;
 			}
@@ -162,7 +152,7 @@ exit_free_list:
 
 static void *stat_seq_start(struct seq_file *s, loff_t *pos)
 {
-	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
+	struct list_head *l = (struct list_head *)s->private;
 
 	/* Prevent from tracer switch or stat_list modification */
 	mutex_lock(&stat_list_mutex);
@@ -171,14 +161,14 @@ static void *stat_seq_start(struct seq_file *s, loff_t *pos)
 	if (!*pos && current_tracer.stat_headers)
 		current_tracer.stat_headers(s);
 
-	return seq_list_start(&l->list, *pos);
+	return seq_list_start(l, *pos);
 }
 
 static void *stat_seq_next(struct seq_file *s, void *p, loff_t *pos)
 {
-	struct trace_stat_list *l = (struct trace_stat_list *)s->private;
+	struct list_head *l = (struct list_head *)s->private;
 
-	return seq_list_next(p, &l->list, pos);
+	return seq_list_next(p, l, pos);
 }
 
 static void stat_seq_stop(struct seq_file *m, void *p)
@@ -188,8 +178,10 @@ static void stat_seq_stop(struct seq_file *m, void *p)
 
 static int stat_seq_show(struct seq_file *s, void *v)
 {
-	struct trace_stat_list *l = list_entry(v, struct trace_stat_list, list);
-	return current_tracer.stat_show(s, l->stat);
+	struct trace_stat_list *entry =
+		list_entry(v, struct trace_stat_list, list);
+
+	return current_tracer.stat_show(s, entry->stat);
 }
 
 static const struct seq_operations trace_stat_seq_ops = {
@@ -237,7 +229,6 @@ static int __init tracing_stat_init(void)
 	struct dentry *d_tracing;
 	struct dentry *entry;
 
-	INIT_LIST_HEAD(&stat_list.list);
 	d_tracing = tracing_init_dentry();
 
 	entry = debugfs_create_file("trace_stat", 0444, d_tracing,
-- 
1.5.6.5

-- 

[PATCH v2 4/4] trace: clean up funny line breaks in stat_seq_show

[Steven Rostedt]

[-- Attachment #1: 0004-trace-clean-up-funny-line-breaks-in-stat_seq_show.patch --]
[-- Type: text/plain, Size: 945 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: clean up

Andrew Morton pointed out that the entry assignment in stat_seq_show
did not need to be done in the declaration, causing funny line breaks.

This patch makes it a bit more pleasing on the eyes.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace_stat.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index 4cb4ff2..f110ce9 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -178,8 +178,9 @@ static void stat_seq_stop(struct seq_file *m, void *p)
 
 static int stat_seq_show(struct seq_file *s, void *v)
 {
-	struct trace_stat_list *entry =
-		list_entry(v, struct trace_stat_list, list);
+	struct trace_stat_list *entry;
+
+	entry =	list_entry(v, struct trace_stat_list, list);
 
 	return current_tracer.stat_show(s, entry->stat);
 }
-- 
1.5.6.5

-- 

Re: [PATCH v2 0/4] ftrace: important updates

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> The first patch is a critical fix that needs to get into 2.6.29.
> 
> The next patch is a rename of tracing_on to writing_enabled since
> tracing_on is confusing, and the file enables or disables writes.
> 
> The last patch is a fix to tip that prevents a memory leak.
> 
> This version I added some clean ups that Andrew Morton suggested.
> I know you did not pull the previous version, because I forgot
> to push the changes to kernel.org ;-)
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: tip/devel
> 
> 
> Frederic Weisbecker (1):
>       tracing/ftrace: fix a memory leak in stat tracing
> 
> Steven Rostedt (3):
>       ftrace: convert unsigned index to signed
>       trace: clean up funny line breaks in stat_seq_show

pulled most of it, thanks Steve, except this bit:

>       ring-buffer: rename debugfs file tracing_on to writing_enabled

writing_enabled is at least as confusing as tracing_on - if not more so.

The user really does not care about all the deeper machinery that happens 
in ftrace - the difference between a 'light' disabling of a tracer and a 
'heavy' disabling of a tracer (which means it unregisters itself 
completely in essence).

To resolve this, we should probably hide this difference altogether (as i 
have suggested to do many months ago, when this first came up), by 
removing tracing_enabled.

A tracer can still be fully unregistered: by simply switching the current 
tracer to the 'nop' tracer. tracing_on/off remains the lightweight version 
that most users are interested in anyway.

	Ingo

Re: [PATCH v2 0/4] ftrace: important updates

[Pekka Paalanen]

On Wed, 7 Jan 2009 10:49:05 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >       ring-buffer: rename debugfs file tracing_on to writing_enabled
> 
> writing_enabled is at least as confusing as tracing_on - if not more so.
> 
> The user really does not care about all the deeper machinery that happens 
> in ftrace - the difference between a 'light' disabling of a tracer and a 
> 'heavy' disabling of a tracer (which means it unregisters itself 
> completely in essence).
> 
> To resolve this, we should probably hide this difference altogether (as i 
> have suggested to do many months ago, when this first came up), by 
> removing tracing_enabled.
> 
> A tracer can still be fully unregistered: by simply switching the current 
> tracer to the 'nop' tracer. tracing_on/off remains the lightweight version 
> that most users are interested in anyway.

This sounds even better. We should not need tracing_enabled anymore, since
the buffer size can be changed regardless. tracing_on is more useful to
mmiotrace than tracing_enabled, too, since mmiotrace does not implement
the proper pausing behaviour on the tracing_enabled trigger.

But if Steven wants to keep the "pause" semantics, should there be a
callback dispatched to the tracer from tracing_on, just like there is
from tracing_enabled currently? Of course the callback semantics would
be different and the usual reaction would be to do nothing. It would
only be meaningful to tracers that update data outside the ring buffer,
so that they can properly pause.

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

Re: [PATCH v2 0/4] ftrace: important updates

[Steven Rostedt]

On Wed, 7 Jan 2009, Pekka Paalanen wrote:

> On Wed, 7 Jan 2009 10:49:05 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > >       ring-buffer: rename debugfs file tracing_on to writing_enabled
> > 
> > writing_enabled is at least as confusing as tracing_on - if not more so.
> > 
> > The user really does not care about all the deeper machinery that happens 
> > in ftrace - the difference between a 'light' disabling of a tracer and a 
> > 'heavy' disabling of a tracer (which means it unregisters itself 
> > completely in essence).
> > 
> > To resolve this, we should probably hide this difference altogether (as i 
> > have suggested to do many months ago, when this first came up), by 
> > removing tracing_enabled.
> > 
> > A tracer can still be fully unregistered: by simply switching the current 
> > tracer to the 'nop' tracer. tracing_on/off remains the lightweight version 
> > that most users are interested in anyway.
> 
> This sounds even better. We should not need tracing_enabled anymore, since
> the buffer size can be changed regardless. tracing_on is more useful to
> mmiotrace than tracing_enabled, too, since mmiotrace does not implement
> the proper pausing behaviour on the tracing_enabled trigger.
> 
> But if Steven wants to keep the "pause" semantics, should there be a
> callback dispatched to the tracer from tracing_on, just like there is
> from tracing_enabled currently? Of course the callback semantics would
> be different and the usual reaction would be to do nothing. It would
> only be meaningful to tracers that update data outside the ring buffer,
> so that they can properly pause.

The tracing_on is implemented by the ring buffer, and disables all ring 
buffers, even those that are not part of ftrace. This file really has no 
concept of a tracer. It simply stops writing to the ring buffer.

Things like the irq latency tracer is will still update its "max time" 
when tracing is off, although the trace output will not be updated.

I could remove tracing_enabled, and move the tracing_on to trace.c,
that just calls the tracing_off of the ring buffer, and then the file will 
be at the higher layer, and have a concept of the current tracer.

-- Steve

Re: [PATCH v2 0/4] ftrace: important updates

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 7 Jan 2009, Pekka Paalanen wrote:
> 
> > On Wed, 7 Jan 2009 10:49:05 +0100
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > >       ring-buffer: rename debugfs file tracing_on to writing_enabled
> > > 
> > > writing_enabled is at least as confusing as tracing_on - if not more so.
> > > 
> > > The user really does not care about all the deeper machinery that happens 
> > > in ftrace - the difference between a 'light' disabling of a tracer and a 
> > > 'heavy' disabling of a tracer (which means it unregisters itself 
> > > completely in essence).
> > > 
> > > To resolve this, we should probably hide this difference altogether (as i 
> > > have suggested to do many months ago, when this first came up), by 
> > > removing tracing_enabled.
> > > 
> > > A tracer can still be fully unregistered: by simply switching the current 
> > > tracer to the 'nop' tracer. tracing_on/off remains the lightweight version 
> > > that most users are interested in anyway.
> > 
> > This sounds even better. We should not need tracing_enabled anymore, since
> > the buffer size can be changed regardless. tracing_on is more useful to
> > mmiotrace than tracing_enabled, too, since mmiotrace does not implement
> > the proper pausing behaviour on the tracing_enabled trigger.
> > 
> > But if Steven wants to keep the "pause" semantics, should there be a
> > callback dispatched to the tracer from tracing_on, just like there is
> > from tracing_enabled currently? Of course the callback semantics would
> > be different and the usual reaction would be to do nothing. It would
> > only be meaningful to tracers that update data outside the ring buffer,
> > so that they can properly pause.
> 
> The tracing_on is implemented by the ring buffer, and disables all ring 
> buffers, even those that are not part of ftrace. This file really has no 
> concept of a tracer. It simply stops writing to the ring buffer.
> 
> Things like the irq latency tracer is will still update its "max time" 
> when tracing is off, although the trace output will not be updated.
> 
> I could remove tracing_enabled, and move the tracing_on to trace.c, that 
> just calls the tracing_off of the ring buffer, and then the file will be 
> at the higher layer, and have a concept of the current tracer.

Sounds good to me. I think the highest utility is in the 'lightweight' 
tracing_on toggle - even though it does not do as thorough of a job of 
disabling a tracer as writing 0 to tracing_on.

I think we shouldnt even _try_ to provide a simple toggle for unregister: 
it is clear from looking at /debug/tracing/current_tracer that the current 
tracer is still active - so asking people to change that back to 'nop' 
will be rather intuitive IMO.

	Ingo