Messages in this thread | | | Date | Tue, 2 Jun 2009 21:01:31 -0400 (EDT) | From | Steven Rostedt <> | Subject | Re: [PATCH] fix bug in ring_buffer_discard_commit |
| |
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 > >
| |