[PATCH] fix bug in ring_buffer_discard_commit

[PATCH] fix bug in ring_buffer_discard_commit

[Tim Bird]

There's a bug in ring_buffer_discard_commit.  The wrong
pointer is being compared in order to check if the event
can be freed from the buffer rather than discarded
(i.e. marked as PAD).

I noticed this when I was working on duration filtering.
The bug is not deadly - it just results in lots of wasted
space in the buffer.  All filtered events are left in
the buffer and marked as discarded, rather than being
removed from the buffer to make space for other events.

Unfortunately, when I fixed this bug, I got errors doing a
filtered function trace.  Multiple TIME_EXTEND
events pile up in the buffer, and trigger the
following loop overage warning in rb_iter_peek():

again:
	...
	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 10))
		return NULL;

I'm not sure what the best way is to fix this. I don't
know if I should extend the loop threshhold, or if I should
make the test more complex (ignore TIME_EXTEND
events), or just get rid of this loop check completely.

Note that if I implement a workaround for this, then I
see another problem from rb_advance_iter().  I haven't
tracked that one down yet.

In general, it seems like the case of removing filtered
events has not been working properly, and so some assumptions
about buffer invariant conditions need to be revisited.

Here's the patch for the simple fix:

Compare correct pointer for checking if an event can be
freed rather than left as discarded in the buffer.

Signed-off-by: Tim Bird <tim.bird@am.sony.com
---
 kernel/trace/ring_buffer.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1711,7 +1711,7 @@ void ring_buffer_discard_commit(struct r

 	bpage = cpu_buffer->tail_page;

-	if (bpage == (void *)addr && rb_page_write(bpage) == old_index) {
+	if (bpage->page == (void *)addr && rb_page_write(bpage) == old_index) {
 		/*
 		 * This is on the tail page. It is possible that
 		 * a write could come in and move the tail page

Re: [PATCH] fix bug in ring_buffer_discard_commit

[Steven Rostedt]

On Tue, 2 Jun 2009, Tim Bird wrote:

> There's a bug in ring_buffer_discard_commit.  The wrong
> pointer is being compared in order to check if the event
> can be freed from the buffer rather than discarded
> (i.e. marked as PAD).
> 
> I noticed this when I was working on duration filtering.
> The bug is not deadly - it just results in lots of wasted
> space in the buffer.  All filtered events are left in
> the buffer and marked as discarded, rather than being
> removed from the buffer to make space for other events.
> 
> Unfortunately, when I fixed this bug, I got errors doing a
> filtered function trace.  Multiple TIME_EXTEND
> events pile up in the buffer, and trigger the
> following loop overage warning in rb_iter_peek():
> 
> again:
> 	...
> 	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 10))
> 		return NULL;
> 
> I'm not sure what the best way is to fix this. I don't
> know if I should extend the loop threshhold, or if I should
> make the test more complex (ignore TIME_EXTEND
> events), or just get rid of this loop check completely.
> 
> Note that if I implement a workaround for this, then I
> see another problem from rb_advance_iter().  I haven't
> tracked that one down yet.
> 
> In general, it seems like the case of removing filtered
> events has not been working properly, and so some assumptions
> about buffer invariant conditions need to be revisited.
> 
> Here's the patch for the simple fix:
> 
> Compare correct pointer for checking if an event can be
> freed rather than left as discarded in the buffer.
> 
> Signed-off-by: Tim Bird <tim.bird@am.sony.com

Thanks Tim! I'll queue it up.

I'll take a look at the code to see what other issues might be happening.

-- Steve

> ---
>  kernel/trace/ring_buffer.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1711,7 +1711,7 @@ void ring_buffer_discard_commit(struct r
> 
>  	bpage = cpu_buffer->tail_page;
> 
> -	if (bpage == (void *)addr && rb_page_write(bpage) == old_index) {
> +	if (bpage->page == (void *)addr && rb_page_write(bpage) == old_index) {
>  		/*
>  		 * This is on the tail page. It is possible that
>  		 * a write could come in and move the tail page
> 
> 

[tip:tracing/core] ring-buffer: fix bug in ring_buffer_discard_commit

[tip-bot for Tim Bird]

Commit-ID:  a2023556409cf7fec5d67a26f7fcfa57c5a4086d
Gitweb:     http://git.kernel.org/tip/a2023556409cf7fec5d67a26f7fcfa57c5a4086d
Author:     Tim Bird <tim.bird@am.sony.com>
AuthorDate: Tue, 2 Jun 2009 17:06:54 -0700
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 3 Jun 2009 10:15:06 -0400

ring-buffer: fix bug in ring_buffer_discard_commit

There's a bug in ring_buffer_discard_commit.  The wrong
pointer is being compared in order to check if the event
can be freed from the buffer rather than discarded
(i.e. marked as PAD).

I noticed this when I was working on duration filtering.
The bug is not deadly - it just results in lots of wasted
space in the buffer.  All filtered events are left in
the buffer and marked as discarded, rather than being
removed from the buffer to make space for other events.

Unfortunately, when I fixed this bug, I got errors doing a
filtered function trace.  Multiple TIME_EXTEND
events pile up in the buffer, and trigger the
following loop overage warning in rb_iter_peek():

again:
	...
	if (RB_WARN_ON(cpu_buffer, ++nr_loops > 10))
		return NULL;

I'm not sure what the best way is to fix this. I don't
know if I should extend the loop threshhold, or if I should
make the test more complex (ignore TIME_EXTEND
events), or just get rid of this loop check completely.

Note that if I implement a workaround for this, then I
see another problem from rb_advance_iter().  I haven't
tracked that one down yet.

In general, it seems like the case of removing filtered
events has not been working properly, and so some assumptions
about buffer invariant conditions need to be revisited.

Here's the patch for the simple fix:

Compare correct pointer for checking if an event can be
freed rather than left as discarded in the buffer.

Signed-off-by: Tim Bird <tim.bird@am.sony.com>
LKML-Reference: <4A25BE9E.5090909@am.sony.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 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 16b24d4..9453023 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1708,7 +1708,7 @@ void ring_buffer_discard_commit(struct ring_buffer *buffer,
 
 	bpage = cpu_buffer->tail_page;
 
-	if (bpage == (void *)addr && rb_page_write(bpage) == old_index) {
+	if (bpage->page == (void *)addr && rb_page_write(bpage) == old_index) {
 		/*
 		 * This is on the tail page. It is possible that
 		 * a write could come in and move the tail page