[PATCH 0/2] ftrace updates for 2.6.28

[PATCH 0/2] ftrace updates for 2.6.28

[Steven Rostedt]

Stephen and Ingo,

The following patches are needed for 2.6.28. The first one solves a bug
where the resizing of the buffer array was not protected against
modifications by ftrace.

The second patch solves an issue tha Thomas Gleixner was seeing where
the tracer would detect the infinite recursion too easily by
tracing the timer code. This patch prevents the loop from happening
and still does not lose any trace entries.

The changes caused by these patches are low in risk, but I would
still like to put them through linux-next before passing them
off to Linus.

Note, these changes are based against Linus's mainline and not tip.
Also note that the branch in this tree is "devel" and not "tip/devel".

The following patches are in:

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

    branch: devel


Steven Rostedt (2):
      ftrace: disable tracing on resize
      ring-buffer: prevent infinite looping on time stamping

----
 kernel/trace/ring_buffer.c |    2 +-
 kernel/trace/trace.c       |   17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

[PATCH 1/2] ftrace: disable tracing on resize

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-disable-tracing-on-resize.patch --]
[-- Type: text/plain, Size: 1805 bytes --]

Impact: fix for bug on resize

This patch addresses the bug found here:

 http://bugzilla.kernel.org/show_bug.cgi?id=11996

When ftrace converted to the new unified trace buffer, the resizing of
the buffer was not protected as much as it was originally. If tracing
is performed while the resize occurs, then the buffer can be corrupted.

This patch disables all ftrace buffer modifications before a resize
takes place.

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

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 9f3b478..abfa810 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2676,7 +2676,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 {
 	unsigned long val;
 	char buf[64];
-	int ret;
+	int ret, cpu;
 	struct trace_array *tr = filp->private_data;
 
 	if (cnt >= sizeof(buf))
@@ -2704,6 +2704,14 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 		goto out;
 	}
 
+	/* disable all cpu buffers */
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_inc(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_inc(&max_tr.data[cpu]->disabled);
+	}
+
 	if (val != global_trace.entries) {
 		ret = ring_buffer_resize(global_trace.buffer, val);
 		if (ret < 0) {
@@ -2735,6 +2743,13 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 	if (tracing_disabled)
 		cnt = -ENOMEM;
  out:
+	for_each_tracing_cpu(cpu) {
+		if (global_trace.data[cpu])
+			atomic_dec(&global_trace.data[cpu]->disabled);
+		if (max_tr.data[cpu])
+			atomic_dec(&max_tr.data[cpu]->disabled);
+	}
+
 	max_tr.entries = global_trace.entries;
 	mutex_unlock(&trace_types_lock);
 
-- 
1.5.6.5

-- 

Re: [PATCH 1/2] ftrace: disable tracing on resize

[Steven Rostedt]

Pekka,

I forgot to add you to the Cc list. Could you test this patch to see if it 
fixes the bug you reported. It fixed a bug I was able to reproduce, but I 
was never able to reproduce the exact same bug you have seen.

-- Steve


On Mon, 10 Nov 2008, Steven Rostedt wrote:

> Impact: fix for bug on resize
> 
> This patch addresses the bug found here:
> 
>  http://bugzilla.kernel.org/show_bug.cgi?id=11996
> 
> When ftrace converted to the new unified trace buffer, the resizing of
> the buffer was not protected as much as it was originally. If tracing
> is performed while the resize occurs, then the buffer can be corrupted.
> 
> This patch disables all ftrace buffer modifications before a resize
> takes place.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/trace.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 9f3b478..abfa810 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2676,7 +2676,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  {
>  	unsigned long val;
>  	char buf[64];
> -	int ret;
> +	int ret, cpu;
>  	struct trace_array *tr = filp->private_data;
>  
>  	if (cnt >= sizeof(buf))
> @@ -2704,6 +2704,14 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  		goto out;
>  	}
>  
> +	/* disable all cpu buffers */
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_inc(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_inc(&max_tr.data[cpu]->disabled);
> +	}
> +
>  	if (val != global_trace.entries) {
>  		ret = ring_buffer_resize(global_trace.buffer, val);
>  		if (ret < 0) {
> @@ -2735,6 +2743,13 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
>  	if (tracing_disabled)
>  		cnt = -ENOMEM;
>   out:
> +	for_each_tracing_cpu(cpu) {
> +		if (global_trace.data[cpu])
> +			atomic_dec(&global_trace.data[cpu]->disabled);
> +		if (max_tr.data[cpu])
> +			atomic_dec(&max_tr.data[cpu]->disabled);
> +	}
> +
>  	max_tr.entries = global_trace.entries;
>  	mutex_unlock(&trace_types_lock);
>  
> -- 
> 1.5.6.5
> 
> -- 
> 
> 

Re: [PATCH 1/2] ftrace: disable tracing on resize

[Pekka Paalanen]

On Mon, 10 Nov 2008 22:18:19 -0500 (EST)
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Pekka,
> 
> I forgot to add you to the Cc list. Could you test this patch to see if it 
> fixes the bug you reported. It fixed a bug I was able to reproduce, but I 
> was never able to reproduce the exact same bug you have seen.

Thanks, Steve.

I didn't have time to try it myself yet, but a kind soul tried it for me.
He used 2.6.28-rc4 and was immediately able to trigger the bug by just:
cd /debug/tracing
echo 0 > tracing_enabled
echo 200 > trace_entries

Then he applied the patch you referred to, and the bug is still present.
He has a 32-bit x86 amd system, and I'm on x86_64.

I don't know why you couldn't reproduce it, maybe you have all the tracers
enabled in the kernel config, when we only have MMIOTRACE.

I can't promise to look into this deeper in the near future :-/


> On Mon, 10 Nov 2008, Steven Rostedt wrote:
> 
> > Impact: fix for bug on resize
> > 
> > This patch addresses the bug found here:
> > 
> >  http://bugzilla.kernel.org/show_bug.cgi?id=11996
> > 
> > When ftrace converted to the new unified trace buffer, the resizing of
> > the buffer was not protected as much as it was originally. If tracing
> > is performed while the resize occurs, then the buffer can be corrupted.
> > 
> > This patch disables all ftrace buffer modifications before a resize
> > takes place.
> > 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > ---
> >  kernel/trace/trace.c |   17 ++++++++++++++++-
> >  1 files changed, 16 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 9f3b478..abfa810 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -2676,7 +2676,7 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
> >  {
> >  	unsigned long val;
> >  	char buf[64];
> > -	int ret;
> > +	int ret, cpu;
> >  	struct trace_array *tr = filp->private_data;
> >  
> >  	if (cnt >= sizeof(buf))
> > @@ -2704,6 +2704,14 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
> >  		goto out;
> >  	}
> >  
> > +	/* disable all cpu buffers */
> > +	for_each_tracing_cpu(cpu) {
> > +		if (global_trace.data[cpu])
> > +			atomic_inc(&global_trace.data[cpu]->disabled);
> > +		if (max_tr.data[cpu])
> > +			atomic_inc(&max_tr.data[cpu]->disabled);
> > +	}
> > +
> >  	if (val != global_trace.entries) {
> >  		ret = ring_buffer_resize(global_trace.buffer, val);
> >  		if (ret < 0) {
> > @@ -2735,6 +2743,13 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
> >  	if (tracing_disabled)
> >  		cnt = -ENOMEM;
> >   out:
> > +	for_each_tracing_cpu(cpu) {
> > +		if (global_trace.data[cpu])
> > +			atomic_dec(&global_trace.data[cpu]->disabled);
> > +		if (max_tr.data[cpu])
> > +			atomic_dec(&max_tr.data[cpu]->disabled);
> > +	}
> > +
> >  	max_tr.entries = global_trace.entries;
> >  	mutex_unlock(&trace_types_lock);
> >  
> > -- 
> > 1.5.6.5

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

Re: [PATCH 1/2] ftrace: disable tracing on resize

[Steven Rostedt]

On Wed, 12 Nov 2008, Pekka Paalanen wrote:

> On Mon, 10 Nov 2008 22:18:19 -0500 (EST)
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Pekka,
> > 
> > I forgot to add you to the Cc list. Could you test this patch to see if it 
> > fixes the bug you reported. It fixed a bug I was able to reproduce, but I 
> > was never able to reproduce the exact same bug you have seen.
> 
> Thanks, Steve.
> 
> I didn't have time to try it myself yet, but a kind soul tried it for me.
> He used 2.6.28-rc4 and was immediately able to trigger the bug by just:
> cd /debug/tracing
> echo 0 > tracing_enabled
> echo 200 > trace_entries
> 
> Then he applied the patch you referred to, and the bug is still present.
> He has a 32-bit x86 amd system, and I'm on x86_64.
> 
> I don't know why you couldn't reproduce it, maybe you have all the tracers
> enabled in the kernel config, when we only have MMIOTRACE.
> 
> I can't promise to look into this deeper in the near future :-/
> 

OK, I'll disable everything but MMIOTRACE and see if I can reproduce it.

Thanks,

-- Steve

Re: [PATCH 1/2] ftrace: disable tracing on resize

[Steven Rostedt]

On Wed, 12 Nov 2008, Pekka Paalanen wrote:

> On Mon, 10 Nov 2008 22:18:19 -0500 (EST)
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > Pekka,
> > 
> > I forgot to add you to the Cc list. Could you test this patch to see if it 
> > fixes the bug you reported. It fixed a bug I was able to reproduce, but I 
> > was never able to reproduce the exact same bug you have seen.
> 
> Thanks, Steve.
> 
> I didn't have time to try it myself yet, but a kind soul tried it for me.
> He used 2.6.28-rc4 and was immediately able to trigger the bug by just:
> cd /debug/tracing
> echo 0 > tracing_enabled
> echo 200 > trace_entries
> 
> Then he applied the patch you referred to, and the bug is still present.
> He has a 32-bit x86 amd system, and I'm on x86_64.
> 
> I don't know why you couldn't reproduce it, maybe you have all the tracers
> enabled in the kernel config, when we only have MMIOTRACE.

After disabling all other tracers, you are right. This sequence crashes.

I'll look into it, and I should have a patch sometime tonight.

-- Steve

[PATCH 2/2] ring-buffer: prevent infinite looping on time stamping

[Steven Rostedt]

[-- Attachment #1: 0002-ring-buffer-prevent-infinite-looping-on-time-stampi.patch --]
[-- Type: text/plain, Size: 2020 bytes --]

Impact: removal of unnecessary looping

The lockless part of the ring buffer allows for reentry into the code
from interrupts. A timestamp is taken, a test is preformed and if it
detects that an interrupt occurred that did tracing, it tries again.

The problem arises if the timestamp code itself causes a trace.
The detection will detect this and loop again. The difference between
this and an interrupt doing tracing, is that this will fail every time,
and cause an infinite loop.

Currently, we test if the loop happens 1000 times, and if so, it will
produce a warning and disable the ring buffer.

The problem with this approach is that it makes it difficult to perform
some types of tracing (tracing the timestamp code itself).

Each trace entry has a delta timestamp from the previous entry.
If a trace entry is reserved but and interrupt occurs and traces before
the previous entry is commited, the delta timestamp for that entry will
be zero. This actually makes sense in terms of tracing, because the
interrupt entry happened before the preempted entry was commited, so
one may consider the two happening at the same time. The order is
still preserved in the buffer.

With this idea, instead of trying to get a new timestamp if an interrupt
made it in between the timestamp and the test, the entry could simply
make the delta zero and continue. This will prevent interrupts or
tracers in the timer code from causing the above loop.

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

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 3f33806..2f76193 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1060,7 +1060,7 @@ rb_reserve_next_event(struct ring_buffer_per_cpu *cpu_buffer,
 
 		/* Did the write stamp get updated already? */
 		if (unlikely(ts < cpu_buffer->write_stamp))
-			goto again;
+			delta = 0;
 
 		if (test_time_stamp(delta)) {
 
-- 
1.5.6.5

-- 

Re: [PATCH 0/2] ftrace updates for 2.6.28

[Steven Rostedt]

On Mon, 10 Nov 2008, Steven Rostedt wrote:
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: devel
> 
> 
> Steven Rostedt (2):
>       ftrace: disable tracing on resize
>       ring-buffer: prevent infinite looping on time stamping
> 

Ingo,

I just pushed these two patches to tip/devel after solving a conflict with 
the first one.

-- Steve

Re: [PATCH 0/2] ftrace updates for 2.6.28

[Ingo Molnar]

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

> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: devel
> 
> Steven Rostedt (2):
>       ftrace: disable tracing on resize
>       ring-buffer: prevent infinite looping on time stamping
> 
> ----
>  kernel/trace/ring_buffer.c |    2 +-
>  kernel/trace/trace.c       |   17 ++++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)

pulled into tip/tracing/urgent, thanks Steve!

> The following patches are needed for 2.6.28. The first one solves a 
> bug where the resizing of the buffer array was not protected against 
> modifications by ftrace.
> 
> The second patch solves an issue tha Thomas Gleixner was seeing 
> where the tracer would detect the infinite recursion too easily by 
> tracing the timer code. This patch prevents the loop from happening 
> and still does not lose any trace entries.
> 
> The changes caused by these patches are low in risk, but I would 
> still like to put them through linux-next before passing them off to 
> Linus.
> 
> Note, these changes are based against Linus's mainline and not tip. 
> Also note that the branch in this tree is "devel" and not 
> "tip/devel".

that's OK. I resolved the (trivial) conflict with pending changes in 
tip/tracing/ftrace and pushed out a new ftrace integration branch 
towards linux-next.

	Ingo