Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.

[Gilboa Davara <gilboad@gmail.com>]

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