[PATCH 0/2] ftrace: updates for linux-tip - Kernel

This is a discussion on [PATCH 0/2] ftrace: updates for linux-tip - Kernel ; Ingo, The following patches are in: git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git branch: tip/devel Steven Rostedt (2): ftrace: prevent ftrace_special from recursion ring-buffer: replace most bug ons with warn on and disable buffer ---- kernel/trace/ring_buffer.c | 65 +++++++++++++++++++++++++++++++++----------- kernel/trace/trace.c | 8 +++-- 2 files changed, ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [PATCH 0/2] ftrace: updates for linux-tip

  1. [PATCH 0/2] ftrace: updates for linux-tip

    Ingo,

    The following patches are in:

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

    branch: tip/devel


    Steven Rostedt (2):
    ftrace: prevent ftrace_special from recursion
    ring-buffer: replace most bug ons with warn on and disable buffer

    ----
    kernel/trace/ring_buffer.c | 65 +++++++++++++++++++++++++++++++++-----------
    kernel/trace/trace.c | 8 +++--
    2 files changed, 54 insertions(+), 19 deletions(-)
    --
    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. [PATCH 2/2] ring-buffer: replace most bug ons with warn on and disable buffer

    This patch replaces most of the BUG_ONs in the ring_buffer code with
    RB_WARN_ON variants. It adds some more variants as needed for the
    replacement. This lets the buffer die nicely and still warn the user.

    One BUG_ON remains in the code, and that is because it detects a
    bad pointer passed in by the calling function, and not a bug by
    the ring buffer code itself.

    Signed-off-by: Steven Rostedt
    ---
    kernel/trace/ring_buffer.c | 65 +++++++++++++++++++++++++++++++++-----------
    1 files changed, 49 insertions(+), 16 deletions(-)

    diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
    index ee9b93d..a6b8f9d 100644
    --- a/kernel/trace/ring_buffer.c
    +++ b/kernel/trace/ring_buffer.c
    @@ -188,6 +188,7 @@ struct ring_buffer_iter {
    u64 read_stamp;
    };

    +/* buffer may be either ring_buffer or ring_buffer_per_cpu */
    #define RB_WARN_ON(buffer, cond) \
    do { \
    if (unlikely(cond)) { \
    @@ -201,10 +202,28 @@ struct ring_buffer_iter {
    if (unlikely(cond)) { \
    atomic_inc(&buffer->record_disabled); \
    WARN_ON(1); \
    + return; \
    + } \
    + } while (0)
    +
    +#define RB_WARN_ON_RET_INT(buffer, cond) \
    + do { \
    + if (unlikely(cond)) { \
    + atomic_inc(&buffer->record_disabled); \
    + WARN_ON(1); \
    return -1; \
    } \
    } while (0)

    +#define RB_WARN_ON_RET_NULL(buffer, cond) \
    + do { \
    + if (unlikely(cond)) { \
    + atomic_inc(&buffer->record_disabled); \
    + WARN_ON(1); \
    + return NULL; \
    + } \
    + } while (0)
    +
    #define RB_WARN_ON_ONCE(buffer, cond) \
    do { \
    static int once; \
    @@ -215,6 +234,17 @@ struct ring_buffer_iter {
    } \
    } while (0)

    +/* buffer must be ring_buffer not per_cpu */
    +#define RB_WARN_ON_UNLOCK(buffer, cond) \
    + do { \
    + if (unlikely(cond)) { \
    + mutex_unlock(&buffer->mutex); \
    + atomic_inc(&buffer->record_disabled); \
    + WARN_ON(1); \
    + return -1; \
    + } \
    + } while (0)
    +
    /**
    * check_pages - integrity check of buffer pages
    * @cpu_buffer: CPU buffer with pages to test
    @@ -227,13 +257,13 @@ static int rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer)
    struct list_head *head = &cpu_buffer->pages;
    struct buffer_page *page, *tmp;

    - RB_WARN_ON_RET(cpu_buffer, head->next->prev != head);
    - RB_WARN_ON_RET(cpu_buffer, head->prev->next != head);
    + RB_WARN_ON_RET_INT(cpu_buffer, head->next->prev != head);
    + RB_WARN_ON_RET_INT(cpu_buffer, head->prev->next != head);

    list_for_each_entry_safe(page, tmp, head, list) {
    - RB_WARN_ON_RET(cpu_buffer,
    + RB_WARN_ON_RET_INT(cpu_buffer,
    page->list.next->prev != &page->list);
    - RB_WARN_ON_RET(cpu_buffer,
    + RB_WARN_ON_RET_INT(cpu_buffer,
    page->list.prev->next != &page->list);
    }

    @@ -440,13 +470,13 @@ rb_remove_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned nr_pages)
    synchronize_sched();

    for (i = 0; i < nr_pages; i++) {
    - BUG_ON(list_empty(&cpu_buffer->pages));
    + RB_WARN_ON_RET(cpu_buffer, list_empty(&cpu_buffer->pages));
    p = cpu_buffer->pages.next;
    page = list_entry(p, struct buffer_page, list);
    list_del_init(&page->list);
    free_buffer_page(page);
    }
    - BUG_ON(list_empty(&cpu_buffer->pages));
    + RB_WARN_ON_RET(cpu_buffer, list_empty(&cpu_buffer->pages));

    rb_reset_cpu(cpu_buffer);

    @@ -468,7 +498,7 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer,
    synchronize_sched();

    for (i = 0; i < nr_pages; i++) {
    - BUG_ON(list_empty(pages));
    + RB_WARN_ON_RET(cpu_buffer, list_empty(pages));
    p = pages->next;
    page = list_entry(p, struct buffer_page, list);
    list_del_init(&page->list);
    @@ -523,7 +553,7 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
    if (size < buffer_size) {

    /* easy case, just free pages */
    - BUG_ON(nr_pages >= buffer->pages);
    + RB_WARN_ON_UNLOCK(buffer, nr_pages >= buffer->pages);

    rm_pages = buffer->pages - nr_pages;

    @@ -542,7 +572,8 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
    * add these pages to the cpu_buffers. Otherwise we just free
    * them all and return -ENOMEM;
    */
    - BUG_ON(nr_pages <= buffer->pages);
    + RB_WARN_ON_UNLOCK(buffer, nr_pages <= buffer->pages);
    +
    new_pages = nr_pages - buffer->pages;

    for_each_buffer_cpu(buffer, cpu) {
    @@ -565,7 +596,7 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
    rb_insert_pages(cpu_buffer, &pages, new_pages);
    }

    - BUG_ON(!list_empty(&pages));
    + RB_WARN_ON_UNLOCK(buffer, !list_empty(&pages));

    out:
    buffer->pages = nr_pages;
    @@ -653,7 +684,7 @@ static void rb_update_overflow(struct ring_buffer_per_cpu *cpu_buffer)
    head += rb_event_length(event)) {

    event = __rb_page_index(cpu_buffer->head_page, head);
    - BUG_ON(rb_null_event(event));
    + RB_WARN_ON_RET(cpu_buffer, rb_null_event(event));
    /* Only count data entries */
    if (event->type != RINGBUF_TYPE_DATA)
    continue;
    @@ -940,7 +971,7 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,

    /* We reserved something on the buffer */

    - BUG_ON(write > BUF_PAGE_SIZE);
    + RB_WARN_ON_RET_NULL(cpu_buffer, write > BUF_PAGE_SIZE);

    event = __rb_page_index(tail_page, tail);
    rb_update_event(event, type, length);
    @@ -1621,7 +1652,7 @@ static void rb_advance_reader(struct ring_buffer_per_cpu *cpu_buffer)
    reader = rb_get_reader_page(cpu_buffer);

    /* This function should not be called when buffer is empty */
    - BUG_ON(!reader);
    + RB_WARN_ON_RET(cpu_buffer, !reader);

    event = rb_reader_event(cpu_buffer);

    @@ -1648,7 +1679,8 @@ static void rb_advance_iter(struct ring_buffer_iter *iter)
    * Check if we are at the end of the buffer.
    */
    if (iter->head >= rb_page_size(iter->head_page)) {
    - BUG_ON(iter->head_page == cpu_buffer->commit_page);
    + RB_WARN_ON_RET(buffer,
    + iter->head_page == cpu_buffer->commit_page);
    rb_inc_iter(iter);
    return;
    }
    @@ -1661,8 +1693,9 @@ static void rb_advance_iter(struct ring_buffer_iter *iter)
    * This should not be called to advance the header if we are
    * at the tail of the buffer.
    */
    - BUG_ON((iter->head_page == cpu_buffer->commit_page) &&
    - (iter->head + length > rb_commit_index(cpu_buffer)));
    + RB_WARN_ON_RET(cpu_buffer,
    + (iter->head_page == cpu_buffer->commit_page) &&
    + (iter->head + length > rb_commit_index(cpu_buffer)));

    rb_update_iter_read_stamp(iter, event);

    --
    1.5.6.5

    --
    --
    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 0/2] ftrace: updates for linux-tip


    * Steven Rostedt wrote:

    > Ingo,
    >
    > The following patches are in:
    >
    > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
    >
    > branch: tip/devel
    >
    > Steven Rostedt (2):
    > ftrace: prevent ftrace_special from recursion
    > ring-buffer: replace most bug ons with warn on and disable buffer


    i've picked up these commits, 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/

  4. Re: [PATCH 0/2] ftrace: updates for linux-tip


    -tip testing found this new ftrace self-test failure on a 64-bit
    testbox:

    [ 4.381253] calling init_wakeup_tracer+0x0/0x12 @ 1
    [ 4.386298] Testing tracer wakeup: <4>------------[ cut here ]------------
    [ 4.599754] WARNING: at kernel/trace/ring_buffer.c:1655 rb_advance_reader+0x2b/0xa3()
    [ 4.600014] Pid: 1, comm: swapper Not tainted 2.6.28-rc4-tip #51129
    [ 4.600014] Call Trace:
    [ 4.600014] [] warn_on_slowpath+0x58/0x75
    [ 4.600014] [] ? trace_hardirqs_off+0xd/0xf
    [ 4.600014] [] ? mark_lock+0x1c/0x361
    [ 4.600014] [] ? trace_hardirqs_off+0xd/0xf
    [ 4.600014] [] ? rb_get_reader_page+0x13f/0x159
    [ 4.600014] [] ? trace_hardirqs_off+0xd/0xf
    [ 4.600014] [] ? time_hardirqs_off+0x21/0x26
    [ 4.600014] [] ? rb_get_reader_page+0x13f/0x159
    [ 4.600014] [] ? trace_hardirqs_off+0xd/0xf
    [ 4.600014] [] ? rb_get_reader_page+0x13f/0x159
    [ 4.600014] [] rb_advance_reader+0x2b/0xa3
    [ 4.600014] [] ring_buffer_peek+0xb2/0xdf
    [ 4.600014] [] ring_buffer_consume+0x24/0x4e
    [ 4.600014] [] trace_test_buffer+0x8f/0xcb
    [ 4.600014] [] trace_selftest_startup_wakeup+0xa8/0x113
    [ 4.600014] [] ? wait_for_common+0xcd/0x12d
    [ 4.600014] [] register_tracer+0xe0/0x183
    [ 4.600014] [] ? timespec_to_ktime+0x1a/0x1c
    [ 4.600014] [] ? init_wakeup_tracer+0x0/0x12
    [ 4.600014] [] init_wakeup_tracer+0x10/0x12
    [ 4.600014] [] do_one_initcall+0x5d/0x13d
    [ 4.600014] [] ? _spin_unlock+0x2b/0x2f
    [ 4.600014] [] ? proc_register+0x184/0x199
    [ 4.600014] [] ? create_proc_entry+0x79/0x8f
    [ 4.600014] [] ? register_irq_proc+0x12/0xcf
    [ 4.600014] [] ? sys_quotactl+0x56e/0x5ad
    [ 4.600014] [] kernel_init+0x127/0x17b
    [ 4.600014] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
    [ 4.600014] [] child_rip+0xa/0x11
    [ 4.600014] [] ? __unlazy_fpu+0x9/0x79
    [ 4.600014] [] ? restore_args+0x0/0x30
    [ 4.600014] [] ? up+0x14/0x3e
    [ 4.600014] [] ? kernel_init+0x0/0x17b
    [ 4.600014] [] ? child_rip+0x0/0x11
    [ 4.600014] ---[ end trace a7919e7f17c0a725 ]---
    [ 4.908030] PASSED

    config attached.

    Ingo


  5. Re: [PATCH 2/2] ring-buffer: replace most bug ons with warn on and disable buffer


    * Steven Rostedt wrote:

    > +#define RB_WARN_ON_RET_INT(buffer, cond) \
    > + do { \
    > + if (unlikely(cond)) { \
    > + atomic_inc(&buffer->record_disabled); \
    > + WARN_ON(1); \
    > return -1; \
    > } \


    btw., the _RET() methods are rather ugly as they include an implicit
    code flow change (a 'return' statement).

    Please change it to a more readable form that preserves the code flow,
    something like:

    if (RB_WARN_ON(buffer, cond))
    return -1;

    See kernel/lockdep.c about how to do this cleanly: introduce a
    _single_ global "oh, we are broken" flag which is increased once and
    never decreased again.

    In the case of lockdep that's the debug_locks flag. It's used in
    various code-flow-preserving forms of debug checks:

    ....
    if (DEBUG_LOCKS_WARN_ON(depth >= 20))
    return;
    ....
    if (!debug_locks_off_graph_unlock())
    return NULL;
    ....
    if (!debug_locks_off_graph_unlock())
    return 0;
    ....
    if (!__raw_spin_is_locked(&lockdep_lock))
    return DEBUG_LOCKS_WARN_ON(1);

    etc.

    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