Re: [PATCH 4/4] ring-buffer: change WARN_ON from checking preempt_count to preemptible

Steven Rostedt
2009-05-08T17:24:11Z

On Fri, 8 May 2009, Andrew Morton wrote:

> On Fri, 8 May 2009 06:53:48 -0400 (EDT) Steven Rostedt <rostedt@goodmis.org> wrote:
> > > >
> > > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > > index 3ae5ccf..3611706 100644
> > > > --- a/kernel/trace/ring_buffer.c
> > > > +++ b/kernel/trace/ring_buffer.c
> > > > @@ -1688,7 +1688,7 @@ void ring_buffer_discard_commit(struct ring_buffer *buffer,
> > > > * committed yet. Thus we can assume that preemption
> > > > * is still disabled.
> > > > */
> > > > - RB_WARN_ON(buffer, !preempt_count());
> > > > + RB_WARN_ON(buffer, preemptible());
> > > >
> > > > cpu = smp_processor_id();
> > > > cpu_buffer = buffer->buffers[cpu];
> > >
> > > smp_processor_id() will warn too.
> > >
> >
> > The difference is that RB_WARN_ON also disables the ring buffer.
> >
>
> Yes, but the kernel will produce two large warning spews for the
> same thing.

Hmm, I should go back to my original patch. Which was:

RB_WARN_ON_PREEMPT(buffer, !preempt_count());

and have at the top of the file:

#ifdef CONFIG_PREEMPT
# RB_WARN_ON_PREEMPT(buffer, cond) RB_WARN_ON(buffer, cond)
#else
# RB_WARN_ON_PREEMPT(buffer, cond) do { } while (0)
#endif

The difference here is not that preemption must be off, but that this must
be called from after a ring_buffer_lock_reserve() but not after a
ring_buffer_unlock_commit().

We have two methods to discard a change, one is to discard it instead of
commiting it. This will try to totally remove the change without leaving
holes. The ring buffer is reentrant, and if an interrupt were to happen
in between reserve and commit, it could store data after this commit. In
which case we have no choice but to leave a hole. But if that did not
happen, then we use cmpxchg to reset the tail of the buffer back to the
original and not have to worry about NMIs and interrupts.

The other version of discarding can be done after a commit. But it only
changes the type of the data and a hole exits. If we did this for all
discards, the ring buffer would be wasted with discarded material and the
filtering would be useless.

The original code only had the discard with a hole, and that was done
after the commit. After converting it over, to the new method, several
places still had the discard after the commit, and that broke the ring
buffer.

There's no easy way to catch this, and do it fast. Since preemption is
always disabled between reserve and commit, I was able to find places that
broke this (and even new places that were added) by simply checking if
preemption is disabled. Note, preemptible() is actually wrong because it
also checks if interrupts are disabled, and if interrupts are disabled but
preemption is not, we miss the case as well.

In any case, if this happens then the ring buffer will be corrupted, and
it is best to disable it. Having two big warnings will help have these
errors stick out more :-)

Actually, the way I've noticed it (when it happened on my laptop) was when
I ran the trace and got no output. Then I decided to do a dmesg to see
why. If I just saw garbage (or even worse, a crash) then I would not have
looked right away at the dmesg. A crash would have prevented me from
looking at dmesg if it locked up my laptop.

-- Steve