[PATCH] ring-buffer: add paranoid checks for loops - Kernel

This is a discussion on [PATCH] ring-buffer: add paranoid checks for loops - Kernel ; [ for 2.6.28 ] While writing a new tracer, I had a bug where I caused the ring-buffer to recurse in a bad way. The bug was with the tracer I was writing and not the ring-buffer itself. But it ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: [PATCH] ring-buffer: add paranoid checks for loops

  1. [PATCH] ring-buffer: add paranoid checks for loops


    [ for 2.6.28 ]

    While writing a new tracer, I had a bug where I caused the ring-buffer
    to recurse in a bad way. The bug was with the tracer I was writing
    and not the ring-buffer itself. But it took a long time to find the
    problem.

    This patch adds paranoid checks into the ring-buffer infrastructure
    that will catch bugs of this nature.

    Note: I put the bug back in the tracer and this patch showed the error
    nicely and prevented the lockup.

    Signed-off-by: Steven Rostedt
    ---
    kernel/trace/ring_buffer.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 45 insertions(+)

    Index: linux-tip.git/kernel/trace/ring_buffer.c
    ================================================== =================
    --- linux-tip.git.orig/kernel/trace/ring_buffer.c 2008-10-29 12:38:54.000000000 -0400
    +++ linux-tip.git/kernel/trace/ring_buffer.c 2008-10-29 16:11:00.000000000 -0400
    @@ -1022,8 +1022,20 @@ rb_reserve_next_event(struct ring_buffer
    struct ring_buffer_event *event;
    u64 ts, delta;
    int commit = 0;
    + int paranoid = 0;

    again:
    + /*
    + * If we loop here 1,000 times, that means we are either
    + * in an interrupt storm, or we have something buggy.
    + * Bail!
    + */
    + if (unlikely(paranoid > 1000)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    ts = ring_buffer_time_stamp(cpu_buffer->cpu);

    /*
    @@ -1532,10 +1544,21 @@ rb_get_reader_page(struct ring_buffer_pe
    {
    struct buffer_page *reader = NULL;
    unsigned long flags;
    + int paranoid = 0;

    spin_lock_irqsave(&cpu_buffer->lock, flags);

    again:
    + /*
    + * We can call here a couple of times, lets only allow 5.
    + */
    + if (unlikely(paranoid > 4)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + reader = NULL;
    + goto out;
    + }
    + paranoid++;
    +
    reader = cpu_buffer->reader_page;

    /* If there's more to read, return this page */
    @@ -1665,6 +1688,7 @@ ring_buffer_peek(struct ring_buffer *buf
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    struct buffer_page *reader;
    + int paranoid = 0;

    if (!cpu_isset(cpu, buffer->cpumask))
    return NULL;
    @@ -1672,6 +1696,16 @@ ring_buffer_peek(struct ring_buffer *buf
    cpu_buffer = buffer->buffers[cpu];

    again:
    + /*
    + * This could happen a few times, but if more than
    + * 10 times, then something is probably wrong.
    + */
    + if (unlikely(paranoid > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    reader = rb_get_reader_page(cpu_buffer);
    if (!reader)
    return NULL;
    @@ -1722,6 +1756,7 @@ ring_buffer_iter_peek(struct ring_buffer
    struct ring_buffer *buffer;
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    + int paranoid = 0;

    if (ring_buffer_iter_empty(iter))
    return NULL;
    @@ -1730,6 +1765,16 @@ ring_buffer_iter_peek(struct ring_buffer
    buffer = cpu_buffer->buffer;

    again:
    + /*
    + * This could happen a few times, but if more than
    + * 10 times, then something is probably wrong.
    + */
    + if (unlikely(paranoid > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    if (rb_per_cpu_empty(cpu_buffer))
    return NULL;

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] ring-buffer: add paranoid checks for loops


    * Steven Rostedt wrote:

    > [ for 2.6.28 ]
    >
    > While writing a new tracer, I had a bug where I caused the
    > ring-buffer to recurse in a bad way. The bug was with the tracer I
    > was writing and not the ring-buffer itself. But it took a long time
    > to find the problem.
    >
    > This patch adds paranoid checks into the ring-buffer infrastructure
    > that will catch bugs of this nature.
    >
    > Note: I put the bug back in the tracer and this patch showed the error
    > nicely and prevented the lockup.
    >
    > Signed-off-by: Steven Rostedt
    > ---
    > kernel/trace/ring_buffer.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
    > 1 file changed, 45 insertions(+)
    >
    > Index: linux-tip.git/kernel/trace/ring_buffer.c
    > ================================================== =================
    > --- linux-tip.git.orig/kernel/trace/ring_buffer.c 2008-10-29 12:38:54.000000000 -0400
    > +++ linux-tip.git/kernel/trace/ring_buffer.c 2008-10-29 16:11:00.000000000 -0400
    > @@ -1022,8 +1022,20 @@ rb_reserve_next_event(struct ring_buffer
    > struct ring_buffer_event *event;
    > u64 ts, delta;
    > int commit = 0;
    > + int paranoid = 0;
    >
    > again:
    > + /*
    > + * If we loop here 1,000 times, that means we are either
    > + * in an interrupt storm, or we have something buggy.
    > + * Bail!
    > + */
    > + if (unlikely(paranoid > 1000)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + return NULL;
    > + }
    > + paranoid++;
    > +
    > ts = ring_buffer_time_stamp(cpu_buffer->cpu);
    >
    > /*
    > @@ -1532,10 +1544,21 @@ rb_get_reader_page(struct ring_buffer_pe
    > {
    > struct buffer_page *reader = NULL;
    > unsigned long flags;
    > + int paranoid = 0;
    >
    > spin_lock_irqsave(&cpu_buffer->lock, flags);
    >
    > again:
    > + /*
    > + * We can call here a couple of times, lets only allow 5.
    > + */
    > + if (unlikely(paranoid > 4)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + reader = NULL;
    > + goto out;
    > + }
    > + paranoid++;
    > +
    > reader = cpu_buffer->reader_page;
    >
    > /* If there's more to read, return this page */
    > @@ -1665,6 +1688,7 @@ ring_buffer_peek(struct ring_buffer *buf
    > struct ring_buffer_per_cpu *cpu_buffer;
    > struct ring_buffer_event *event;
    > struct buffer_page *reader;
    > + int paranoid = 0;
    >
    > if (!cpu_isset(cpu, buffer->cpumask))
    > return NULL;
    > @@ -1672,6 +1696,16 @@ ring_buffer_peek(struct ring_buffer *buf
    > cpu_buffer = buffer->buffers[cpu];
    >
    > again:
    > + /*
    > + * This could happen a few times, but if more than
    > + * 10 times, then something is probably wrong.
    > + */
    > + if (unlikely(paranoid > 10)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + return NULL;
    > + }
    > + paranoid++;
    > +
    > reader = rb_get_reader_page(cpu_buffer);
    > if (!reader)
    > return NULL;
    > @@ -1722,6 +1756,7 @@ ring_buffer_iter_peek(struct ring_buffer
    > struct ring_buffer *buffer;
    > struct ring_buffer_per_cpu *cpu_buffer;
    > struct ring_buffer_event *event;
    > + int paranoid = 0;
    >
    > if (ring_buffer_iter_empty(iter))
    > return NULL;
    > @@ -1730,6 +1765,16 @@ ring_buffer_iter_peek(struct ring_buffer
    > buffer = cpu_buffer->buffer;
    >
    > again:
    > + /*
    > + * This could happen a few times, but if more than
    > + * 10 times, then something is probably wrong.
    > + */
    > + if (unlikely(paranoid > 10)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + return NULL;
    > + }
    > + paranoid++;
    > +


    hm, all those magic constants look a bit like voodoo and make the
    patch ugly, and people who read this will be confused about the
    purpose for sure.

    But the checks are still worth having in practice. So could you please
    improve the comments, to come up with some tangible calculation that
    leads to these constants?

    For example the '1000' constant, how did you come to that? Could you
    estimate what type of interrupt storm is needed to trigger it falsely?
    So instead of this comment:

    > + * If we loop here 1,000 times, that means we are either
    > + * in an interrupt storm, or we have something buggy.
    > + * Bail!


    something like this might look more acceptable:

    > + * If we loop here 1,000 times, that means we are either
    > + * in an interrupt storm that preempted the same trace-entry
    > + * attempt 1000 times in a row, or we have a bug in the tracer.
    > + * Bail!


    i.e. please exaplain every single magic number there so that it can be
    followed how you got to that number, and what precise effects that
    number has.

    In the cases where you just guessed a number based on experiments,
    please think it through and insert an analysis about the effects of
    that number.

    Would this be doable?

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] ring-buffer: add paranoid checks for loops


    On Thu, 30 Oct 2008, Ingo Molnar wrote:
    >
    > hm, all those magic constants look a bit like voodoo and make the
    > patch ugly, and people who read this will be confused about the
    > purpose for sure.


    Point taken.

    >
    > But the checks are still worth having in practice. So could you please
    > improve the comments, to come up with some tangible calculation that
    > leads to these constants?
    >
    > For example the '1000' constant, how did you come to that? Could you
    > estimate what type of interrupt storm is needed to trigger it falsely?
    > So instead of this comment:


    My original number was 100,000, but I thought that a bit high ;-)
    Since it is OK for an interrupt to preempt this code and perform a trace,
    which would make the condition fail by the one being preempted. The
    likelyhood of an interrupt coming in at that location 1000 times in a row
    seems to be awefully low. It's not enough that a 1000 interrupts come in,
    the task being preempted must loop 1000 times and have a trace interrupt
    cause the condition to fail each time. I'll explain it this way in the
    comments.

    I picked a big number because I can see a traced interrupt that is very
    active causing several interruptions in this code.

    >
    > > + * If we loop here 1,000 times, that means we are either
    > > + * in an interrupt storm, or we have something buggy.
    > > + * Bail!

    >
    > something like this might look more acceptable:
    >
    > > + * If we loop here 1,000 times, that means we are either
    > > + * in an interrupt storm that preempted the same trace-entry
    > > + * attempt 1000 times in a row, or we have a bug in the tracer.
    > > + * Bail!

    >
    > i.e. please exaplain every single magic number there so that it can be
    > followed how you got to that number, and what precise effects that
    > number has.
    >
    > In the cases where you just guessed a number based on experiments,
    > please think it through and insert an analysis about the effects of
    > that number.
    >
    > Would this be doable?


    Again, there are small "allowable" races that causes the code to loop a
    few times. I'll try to explain them a bit better in the comments.
    There's small races between the reader and writer that can hit just right
    to cause a "loop again". But these chances are much smaller than the
    interrupt tracing situation.

    I'll look deeper at the reasons for the races and explain them a bit
    better.

    Thanks,

    -- Steve

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. [PATCH -v2] ring-buffer: add paranoid checks for loops

    [
    Changes since v1:

    Updated comments to be more detailed.
    ]

    While writing a new tracer, I had a bug where I caused the ring-buffer
    to recurse in a bad way. The bug was with the tracer I was writing
    and not the ring-buffer itself. But it took a long time to find the
    problem.

    This patch adds paranoid checks into the ring-buffer infrastructure
    that will catch bugs of this nature.

    Note: I put the bug back in the tracer and this patch showed the error
    nicely and prevented the lockup.

    Signed-off-by: Steven Rostedt
    ---
    kernel/trace/ring_buffer.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 58 insertions(+)

    Index: linux-tip.git/kernel/trace/ring_buffer.c
    ================================================== =================
    --- linux-tip.git.orig/kernel/trace/ring_buffer.c 2008-10-30 11:22:43.000000000 -0400
    +++ linux-tip.git/kernel/trace/ring_buffer.c 2008-10-30 22:50:53.000000000 -0400
    @@ -1022,8 +1022,24 @@ rb_reserve_next_event(struct ring_buffer
    struct ring_buffer_event *event;
    u64 ts, delta;
    int commit = 0;
    + int paranoid = 0;

    again:
    + /*
    + * We allow for interrupts to reenter here and do a trace.
    + * If one does, it will cause this original code to loop
    + * back here. Even with heavy interrupts happening, this
    + * should only happen a few times in a row. If this happens
    + * 1000 times in a row, there must be either an interrupt
    + * storm or we have something buggy.
    + * Bail!
    + */
    + if (unlikely(paranoid > 1000)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    ts = ring_buffer_time_stamp(cpu_buffer->cpu);

    /*
    @@ -1532,10 +1548,24 @@ rb_get_reader_page(struct ring_buffer_pe
    {
    struct buffer_page *reader = NULL;
    unsigned long flags;
    + int paranoid = 0;

    spin_lock_irqsave(&cpu_buffer->lock, flags);

    again:
    + /*
    + * This should normally only loop twice. But because the
    + * start of the reader inserts an empty page, it causes
    + * a case where we will loop three times. There should be no
    + * reason to loop four times (that I know of).
    + */
    + if (unlikely(paranoid > 2)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + reader = NULL;
    + goto out;
    + }
    + paranoid++;
    +
    reader = cpu_buffer->reader_page;

    /* If there's more to read, return this page */
    @@ -1665,6 +1695,7 @@ ring_buffer_peek(struct ring_buffer *buf
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    struct buffer_page *reader;
    + int paranoid = 0;

    if (!cpu_isset(cpu, buffer->cpumask))
    return NULL;
    @@ -1672,6 +1703,19 @@ ring_buffer_peek(struct ring_buffer *buf
    cpu_buffer = buffer->buffers[cpu];

    again:
    + /*
    + * We repeat when a timestamp is encountered. It is possible
    + * to get multiple timestamps from an interrupt entering just
    + * as one timestamp is about to be written. The max times
    + * that this can happen is the number of nested interrupts we
    + * can have. 10 should be more than enough.
    + */
    + if (unlikely(paranoid > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    reader = rb_get_reader_page(cpu_buffer);
    if (!reader)
    return NULL;
    @@ -1722,6 +1766,7 @@ ring_buffer_iter_peek(struct ring_buffer
    struct ring_buffer *buffer;
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    + int paranoid = 0;

    if (ring_buffer_iter_empty(iter))
    return NULL;
    @@ -1730,6 +1775,19 @@ ring_buffer_iter_peek(struct ring_buffer
    buffer = cpu_buffer->buffer;

    again:
    + /*
    + * We repeat when a timestamp is encountered. It is possible
    + * to get multiple timestamps from an interrupt entering just
    + * as one timestamp is about to be written. The max times
    + * that this can happen is the number of nested interrupts we
    + * can have. 10 should be more than enough.
    + */
    + if (unlikely(paranoid > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    + paranoid++;
    +
    if (rb_per_cpu_empty(cpu_buffer))
    return NULL;



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH -v2] ring-buffer: add paranoid checks for loops


    * Steven Rostedt wrote:

    > + /*
    > + * This should normally only loop twice. But because the
    > + * start of the reader inserts an empty page, it causes
    > + * a case where we will loop three times. There should be no
    > + * reason to loop four times (that I know of).
    > + */
    > + if (unlikely(paranoid > 2)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + reader = NULL;
    > + goto out;
    > + }
    > + paranoid++;


    ok, the explanations look nice now.

    A small nit - the above comment suggests that looping 4 times is the
    anomaly - still the test is for paranoid > 2 ?

    > + int paranoid = 0;


    another small nit: i'd suggest to rename 'paranoid' to 'nr_loops' or
    'nr_iterations' or so. It is the _condition_ that signals paranoia,
    not the variable in itself - making the current patch look a bit
    weird.

    > again:
    > + /*
    > + * We repeat when a timestamp is encountered. It is possible
    > + * to get multiple timestamps from an interrupt entering just
    > + * as one timestamp is about to be written. The max times
    > + * that this can happen is the number of nested interrupts we
    > + * can have. 10 should be more than enough.
    > + */
    > + if (unlikely(paranoid > 10)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + return NULL;


    s/10 should be more than enough/Nesting higher than 10 is clearly
    anomalous/

    > + /*
    > + * We repeat when a timestamp is encountered. It is possible
    > + * to get multiple timestamps from an interrupt entering just
    > + * as one timestamp is about to be written. The max times
    > + * that this can happen is the number of nested interrupts we
    > + * can have. 10 should be more than enough.
    > + */
    > + if (unlikely(paranoid > 10)) {
    > + RB_WARN_ON(cpu_buffer, 1);
    > + return NULL;


    ditto.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. [PATCH -v3] ring-buffer: add paranoid checks for loops

    [
    Changes from v2:

    Applied Ingo's comments:

    Rephrased one of the comments.

    Renamed the "paranoid" variable into "nr_loops".
    ]

    While writing a new tracer, I had a bug where I caused the ring-buffer
    to recurse in a bad way. The bug was with the tracer I was writing
    and not the ring-buffer itself. But it took a long time to find the
    problem.

    This patch adds paranoid checks into the ring-buffer infrastructure
    that will catch bugs of this nature.

    Note: I put the bug back in the tracer and this patch showed the error
    nicely and prevented the lockup.

    Signed-off-by: Steven Rostedt
    ---
    kernel/trace/ring_buffer.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 56 insertions(+)

    Index: linux-tip.git/kernel/trace/ring_buffer.c
    ================================================== =================
    --- linux-tip.git.orig/kernel/trace/ring_buffer.c 2008-10-30 11:22:43.000000000 -0400
    +++ linux-tip.git/kernel/trace/ring_buffer.c 2008-10-31 09:50:35.000000000 -0400
    @@ -1022,8 +1022,23 @@ rb_reserve_next_event(struct ring_buffer
    struct ring_buffer_event *event;
    u64 ts, delta;
    int commit = 0;
    + int nr_loops = 0;

    again:
    + /*
    + * We allow for interrupts to reenter here and do a trace.
    + * If one does, it will cause this original code to loop
    + * back here. Even with heavy interrupts happening, this
    + * should only happen a few times in a row. If this happens
    + * 1000 times in a row, there must be either an interrupt
    + * storm or we have something buggy.
    + * Bail!
    + */
    + if (unlikely(++nr_loops > 1000)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    +
    ts = ring_buffer_time_stamp(cpu_buffer->cpu);

    /*
    @@ -1532,10 +1547,23 @@ rb_get_reader_page(struct ring_buffer_pe
    {
    struct buffer_page *reader = NULL;
    unsigned long flags;
    + int nr_loops = 0;

    spin_lock_irqsave(&cpu_buffer->lock, flags);

    again:
    + /*
    + * This should normally only loop twice. But because the
    + * start of the reader inserts an empty page, it causes
    + * a case where we will loop three times. There should be no
    + * reason to loop four times (that I know of).
    + */
    + if (unlikely(++nr_loops > 3)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + reader = NULL;
    + goto out;
    + }
    +
    reader = cpu_buffer->reader_page;

    /* If there's more to read, return this page */
    @@ -1665,6 +1693,7 @@ ring_buffer_peek(struct ring_buffer *buf
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    struct buffer_page *reader;
    + int nr_loops = 0;

    if (!cpu_isset(cpu, buffer->cpumask))
    return NULL;
    @@ -1672,6 +1701,19 @@ ring_buffer_peek(struct ring_buffer *buf
    cpu_buffer = buffer->buffers[cpu];

    again:
    + /*
    + * We repeat when a timestamp is encountered. It is possible
    + * to get multiple timestamps from an interrupt entering just
    + * as one timestamp is about to be written. The max times
    + * that this can happen is the number of nested interrupts we
    + * can have. Nesting 10 deep of interrupts is clearly
    + * an anomaly.
    + */
    + if (unlikely(++nr_loops > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    +
    reader = rb_get_reader_page(cpu_buffer);
    if (!reader)
    return NULL;
    @@ -1722,6 +1764,7 @@ ring_buffer_iter_peek(struct ring_buffer
    struct ring_buffer *buffer;
    struct ring_buffer_per_cpu *cpu_buffer;
    struct ring_buffer_event *event;
    + int nr_loops = 0;

    if (ring_buffer_iter_empty(iter))
    return NULL;
    @@ -1730,6 +1773,19 @@ ring_buffer_iter_peek(struct ring_buffer
    buffer = cpu_buffer->buffer;

    again:
    + /*
    + * We repeat when a timestamp is encountered. It is possible
    + * to get multiple timestamps from an interrupt entering just
    + * as one timestamp is about to be written. The max times
    + * that this can happen is the number of nested interrupts we
    + * can have. Nesting 10 deep of interrupts is clearly
    + * an anomaly.
    + */
    + if (unlikely(++nr_loops > 10)) {
    + RB_WARN_ON(cpu_buffer, 1);
    + return NULL;
    + }
    +
    if (rb_per_cpu_empty(cpu_buffer))
    return NULL;


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH -v2] ring-buffer: add paranoid checks for loops


    On Fri, 31 Oct 2008, Ingo Molnar wrote:
    >
    > > + /*
    > > + * This should normally only loop twice. But because the
    > > + * start of the reader inserts an empty page, it causes
    > > + * a case where we will loop three times. There should be no
    > > + * reason to loop four times (that I know of).
    > > + */
    > > + if (unlikely(paranoid > 2)) {
    > > + RB_WARN_ON(cpu_buffer, 1);
    > > + reader = NULL;
    > > + goto out;
    > > + }
    > > + paranoid++;

    >
    > ok, the explanations look nice now.
    >
    > A small nit - the above comment suggests that looping 4 times is the
    > anomaly - still the test is for paranoid > 2 ?


    Yes, that's because the variable started at 0. So > 2 really means the
    loop iterated more than 3.

    My last patch changed it to be a bit easier to understand...

    if (unlikely(++nr_loops > 3)) {

    -- Steve

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH -v3] ring-buffer: add paranoid checks for loops


    * Steven Rostedt wrote:

    > [
    > Changes from v2:
    >
    > Applied Ingo's comments:
    >
    > Rephrased one of the comments.
    >
    > Renamed the "paranoid" variable into "nr_loops".
    > ]
    >
    > While writing a new tracer, I had a bug where I caused the ring-buffer
    > to recurse in a bad way. The bug was with the tracer I was writing
    > and not the ring-buffer itself. But it took a long time to find the
    > problem.
    >
    > This patch adds paranoid checks into the ring-buffer infrastructure
    > that will catch bugs of this nature.
    >
    > Note: I put the bug back in the tracer and this patch showed the error
    > nicely and prevented the lockup.
    >
    > Signed-off-by: Steven Rostedt
    > ---
    > kernel/trace/ring_buffer.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
    > 1 file changed, 56 insertions(+)


    applied to tip/tracing/urgent, thanks Steve!

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread