lkml.org 
[lkml]   [2007]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
    From
    Date
    On Sat, 2007-09-15 at 18:32 +0530, Satyam Sharma wrote:
    > Hi,
    >
    >
    > On 9/15/07, Gilboa Davara <gilboad@gmail.com> wrote:
    > > Hello all,
    > >
    > > In a small exchange in fedora-kernel-list [1] Eric Sandeen has pointed
    > > out a possible stack overflow... when CONFIG_DEBUG_STACKOVERFLOW is
    > > enabled. (Though not limited to it)
    >
    > Yeah, I have experienced this phenomenon/problem myself.
    >
    >
    > > Code path is simple: do_IRQ detects a a near stack overflow condition
    > > and calls show_trace_log_lvl which, down the line uses __print_symbol
    > > and sprint_symbol to print the call stack.
    > > However, both __print_symbol + sprint_symbol are eating no-less then
    > > 128+223 bytes on static char arrays, which, given the fact that this
    > > code path is actually generated by low stack warning (< 512 bytes),
    > > might turn a minor (?) problem (low stack) into a full blown crash.
    >
    > __print_symbol() and sprint_symbol() are called multiple times during
    > oopsen / panics. I think those buffers were static char arrays for a good
    > reason ...

    OK. Point taken.
    I pull this patch pending some additional thinking.

    > > The patch itself is fairly simple and non-intrusive. [2]
    > > Both functions allocate memory for their buffers - falling back to
    > > minimal address display if memory allocation fails.
    > >
    > > P.S. Can anyone please point me to the maintainer of kernel/syms? (I
    > > rather not spam world + dog for such a minor patch)
    >
    > Anything that touches the panic codepath is important, not minor at all.

    Bad wording on my part.
    By minor I meant, changes a single source file, doesn't change
    interfaces, doesn't change code behavior beyond it's local scope.

    > > [2]. In theory, there's a second option: pre-allocating memory on a
    > > per_cpu basis, however:
    > > A. dump_trace/stack are usually called when something bad has happened -
    > > reducing the need for performance optimizations.
    >
    > That's not a performance optimization -- avoiding repeated kmalloc()'s in the
    > panic codepath sounds like a *requirement* to me.

    ACK.

    Though in my defense, solution [2] requires a massive surgery that would
    have made this patch far more intrusive.

    >
    >
    > > B. per_cpu allocation will also require local_irq_disable/enable as both
    > > functions are being called from multiple contexts. Too much hassle.
    >
    > I think not bothering about any locking in these codepaths may not be an
    > entirely unreasonable thing to do (sorry about the triple negation in the
    > sentence). What I mean is that there are places in these codepaths where
    > we already don't bother with locking ...
    >
    > Overall I don't much like introducing kmalloc(GFP_ATOMIC) in these codepaths
    > and would ask you guys to consider some other pre-allocation (i.e. static
    > allocation not on stack but in .data) alternative instead ...
    >

    > Satyam

    No locking what-so-ever is a bad idea. dump_stack/trace are being called
    by non-fatal sources (sleep while atomic; stack-check; debugging) that
    may produce problematic results if a static/shared buffer is being used
    with no locks.
    We can agree that using in-stack char buffer is very problematic -
    especially given the fact that 4K is becoming the default build option.

    I'll try and create an option 2 (static allocation, minimal locking)
    patch and post ASAP.
    Hopefully it'll fare better. (While keeping the current interface intact
    and reducing the damage/noise)

    - Gilboa

    -
    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/

    \
     
     \ /
      Last update: 2007-09-15 17:17    [W:2.767 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site