[PATCH 0/3] ftrace: important updates

[PATCH 0/3] 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.

-- Steve


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 (2):
      ftrace: convert unsigned index to signed
      ring-buffer: rename debugfs file tracing_on to writing_enabled

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

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

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-convert-unsigned-index-to-signed.patch --]
[-- Type: text/plain, Size: 1214 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.

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..3576707 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;
+	long			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

-- 

Re: [PATCH 1/3] ftrace: convert unsigned index to signed

[Andrew Morton]

On Tue, 06 Jan 2009 17:33:38 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 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.
> 
> 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..3576707 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;
> +	long			index;

Does that actually need to be a long type?

>  	struct dyn_ftrace	records[];
>  };
>  
> @@ -786,7 +786,7 @@ enum {
>  
>  struct ftrace_iterator {
>  	struct ftrace_page	*pg;
> -	unsigned		idx;
> +	int			idx;

because we have

        if (iter->idx >= iter->pg->index) {

Are 32-bit types actually more efficient than 64-bit types on any
64-bit hardware which we care about?  

Re: [PATCH 1/3] ftrace: convert unsigned index to signed

[Steven Rostedt]

On Tue, 6 Jan 2009, Andrew Morton wrote:

> On Tue, 06 Jan 2009 17:33:38 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 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.
> > 
> > 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..3576707 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;
> > +	long			index;
> 
> Does that actually need to be a long type?

No, I think I just automatically typed "unsigned long" before, and here
I just removed the unsigned. It can also be an int. It only indexes what 
is on a page.


> 
> >  	struct dyn_ftrace	records[];
> >  };
> >  
> > @@ -786,7 +786,7 @@ enum {
> >  
> >  struct ftrace_iterator {
> >  	struct ftrace_page	*pg;
> > -	unsigned		idx;
> > +	int			idx;
> 
> because we have
> 
>         if (iter->idx >= iter->pg->index) {
> 
> Are 32-bit types actually more efficient than 64-bit types on any
> 64-bit hardware which we care about?  

They both should always be way less than 2^31, so they both can be int.

Thanks, I'll fix this one up. I don't think Ingo pulled it yet.

-- Steve

[PATCH 2/3] 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

-- 

Re: [PATCH 2/3] ring-buffer: rename debugfs file tracing_on to writing_enabled

[Andrew Morton]

On Tue, 06 Jan 2009 17:33:39 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

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

There's no corresponding documentation update needed?

Re: [PATCH 2/3] ring-buffer: rename debugfs file tracing_on to writing_enabled

[Steven Rostedt]

On Tue, 6 Jan 2009, Andrew Morton wrote:

> On Tue, 06 Jan 2009 17:33:39 -0500
> 
> There's no corresponding documentation update needed?

I've fallen a little behind in the documentation. I wanted to see what got 
pulled in before updating ftrace.txt. But you are correct, I probably 
should add the documentation updates along with the patches.

But unfortunately, I'm a bit lazy. I like to wait and see what makes it to 
mainline, and then write a patch to update ftrace.txt to reflect that. It 
lets me do it in one sitting, instead of doing it for each change.

Honestly, I think I document it better that way. If I document right after 
writing the code, I do it with too much knowledge of the code, and my 
writing is confusing for others. If I wait to document, I forget what I 
did, and have to relearn what I wrote, and then I have a better 
perspective of what others will want to know.

Thanks,

-- Steve

[PATCH 3/3] 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

-- 

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

[Andrew Morton]

On Tue, 06 Jan 2009 17:33:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

>  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);
>  }

Rather than jumping through weird 80-column hoops, one can just do:

	struct trace_stat_list *entry;

	...

	entry = list_entry(v, struct trace_stat_list, list);

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

[Steven Rostedt]

On Tue, 6 Jan 2009, Andrew Morton wrote:

> On Tue, 06 Jan 2009 17:33:40 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  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);
> >  }
> 
> Rather than jumping through weird 80-column hoops, one can just do:
> 
> 	struct trace_stat_list *entry;
> 
> 	...
> 
> 	entry = list_entry(v, struct trace_stat_list, list);

OK, this is Frederics work. I'll submit a separate clean up patch.

-- Steve