Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Fri, 2007-09-21 at 10:47 -0400, Steven Rostedt wrote: > On Sat, Sep 15, 2007 at 02:35:29PM +0300, Gilboa Davara wrote: > > - return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size); > > + if (namebuf) > > + kfree(namebuf); > > Note, the if condition is not needed, kfree handles null pointers fine. > > -- Steve Steve, This patch has been scrapped and by another, (hopefully) less problematic one [1]. Thanks for the comments, Gilboa [1] http://lkml.org/lkml/2007/9/21/180 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Sat, Sep 15, 2007 at 02:35:29PM +0300, Gilboa Davara wrote: > - return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size); > + if (namebuf) > + kfree(namebuf); Note, the if condition is not needed, kfree handles null pointers fine. -- Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Sat, Sep 15, 2007 at 02:35:29PM +0300, Gilboa Davara wrote: - return sprintf(buffer, %s+%#lx/%#lx, name, offset, size); + if (namebuf) + kfree(namebuf); Note, the if condition is not needed, kfree handles null pointers fine. -- Steve - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Fri, 2007-09-21 at 10:47 -0400, Steven Rostedt wrote: On Sat, Sep 15, 2007 at 02:35:29PM +0300, Gilboa Davara wrote: - return sprintf(buffer, %s+%#lx/%#lx, name, offset, size); + if (namebuf) + kfree(namebuf); Note, the if condition is not needed, kfree handles null pointers fine. -- Steve Steve, This patch has been scrapped and by another, (hopefully) less problematic one [1]. Thanks for the comments, Gilboa [1] http://lkml.org/lkml/2007/9/21/180 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Sat, 2007-09-15 at 18:32 +0530, Satyam Sharma wrote: > Hi, > > > On 9/15/07, Gilboa Davara <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
Hi,
On 9/15/07, Gilboa Davara <[EMAIL PROTECTED]> 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 ...
> 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.
> Gilboa Davara <[EMAIL PROTECTED]>
>
> [1]
> http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html
>
> [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.
> 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
> --- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.0 +0300
> +++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
> @@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
> /* Look up a kernel symbol and return it in a text buffer. */
> int sprint_symbol(char *buffer, unsigned long address)
> {
> - char *modname;
> - const char *name;
> unsigned long offset, size;
> - char namebuf[KSYM_NAME_LEN];
> + const char *name = NULL;
> + char *namebuf = NULL;
> + char *modname;
> + int ret;
> +
> +
> + /* Static buffer allocation.
> + Required in-order to reduce stack footprint on
> +do_IRQ/4KSTACK/i386 */
> + namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
> + if (namebuf)
> + name = kallsyms_lookup(address, , ,
> + , namebuf);
>
> - name = kallsyms_lookup(address, , , , namebuf);
> if (!name)
> - return sprintf(buffer, "0x%lx", address);
> + ret = sprintf(buffer, "0x%lx", address);
> + else {
> + if (modname)
> + ret = sprintf(buffer, "%s+%#lx/%#lx [%s]",
> + name, offset, size, modname);
> + else
> + ret = sprintf(buffer, "%s+%#lx/%#lx",
> + name, offset, size);
> + }
>
> - if (modname)
> - return sprintf(buffer, "%s+%#lx/%#lx [%s]", name, offset,
> - size, modname);
> - else
> - return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
> + if (namebuf)
> + kfree(namebuf);
> +
> + return ret;
> }
>
> /* Look up a kernel symbol and print it to the kernel messages. */
> void __print_symbol(const char *fmt, unsigned long address)
> {
> - char buffer[KSYM_SYMBOL_LEN];
> + char *buffer = NULL;
>
> - sprint_symbol(buffer, address);
>
> - printk(fmt, buffer);
> + /* Static buffer allocation.
> + Required in-order to reduce stack footprint on
> +do_IRQ/4KSTACK/i386 */
> + buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
> + if (buffer) {
> + sprint_symbol(buffer, address);
> + printk(fmt, buffer);
> + kfree(buffer);
> + } else {
> + /* Address + '0x' + NULL. */
> + char sbuffer[(BITS_PER_LONG / 4) + 3];
> +
> + /*
[Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
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)
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.
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)
--
Gilboa Davara <[EMAIL PROTECTED]>
[1]
http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html
[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.
B. per_cpu allocation will also require local_irq_disable/enable as both
functions are being called from multiple contexts. Too much hassle.
--- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.0 +0300
+++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
@@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
/* Look up a kernel symbol and return it in a text buffer. */
int sprint_symbol(char *buffer, unsigned long address)
{
- char *modname;
- const char *name;
unsigned long offset, size;
- char namebuf[KSYM_NAME_LEN];
+ const char *name = NULL;
+ char *namebuf = NULL;
+ char *modname;
+ int ret;
+
+
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
+ if (namebuf)
+ name = kallsyms_lookup(address, , ,
+ , namebuf);
- name = kallsyms_lookup(address, , , , namebuf);
if (!name)
- return sprintf(buffer, "0x%lx", address);
+ ret = sprintf(buffer, "0x%lx", address);
+ else {
+ if (modname)
+ ret = sprintf(buffer, "%s+%#lx/%#lx [%s]",
+ name, offset, size, modname);
+ else
+ ret = sprintf(buffer, "%s+%#lx/%#lx",
+ name, offset, size);
+ }
- if (modname)
- return sprintf(buffer, "%s+%#lx/%#lx [%s]", name, offset,
- size, modname);
- else
- return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
+ if (namebuf)
+ kfree(namebuf);
+
+ return ret;
}
/* Look up a kernel symbol and print it to the kernel messages. */
void __print_symbol(const char *fmt, unsigned long address)
{
- char buffer[KSYM_SYMBOL_LEN];
+ char *buffer = NULL;
- sprint_symbol(buffer, address);
- printk(fmt, buffer);
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
+ if (buffer) {
+ sprint_symbol(buffer, address);
+ printk(fmt, buffer);
+ kfree(buffer);
+ } else {
+ /* Address + '0x' + NULL. */
+ char sbuffer[(BITS_PER_LONG / 4) + 3];
+
+ /* Fall-back mode.
+ Memory allocation failed.
+ Convert the address to string and display it. */
+ sprintf(sbuffer, "0x%lx", address);
+ printk(fmt, sbuffer);
+ }
}
/* To avoid using get_symbol_offset for every symbol, we carry prefix
along. */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
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)
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.
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)
--
Gilboa Davara [EMAIL PROTECTED]
[1]
http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html
[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.
B. per_cpu allocation will also require local_irq_disable/enable as both
functions are being called from multiple contexts. Too much hassle.
--- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.0 +0300
+++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
@@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
/* Look up a kernel symbol and return it in a text buffer. */
int sprint_symbol(char *buffer, unsigned long address)
{
- char *modname;
- const char *name;
unsigned long offset, size;
- char namebuf[KSYM_NAME_LEN];
+ const char *name = NULL;
+ char *namebuf = NULL;
+ char *modname;
+ int ret;
+
+
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
+ if (namebuf)
+ name = kallsyms_lookup(address, size, offset,
+ modname, namebuf);
- name = kallsyms_lookup(address, size, offset, modname, namebuf);
if (!name)
- return sprintf(buffer, 0x%lx, address);
+ ret = sprintf(buffer, 0x%lx, address);
+ else {
+ if (modname)
+ ret = sprintf(buffer, %s+%#lx/%#lx [%s],
+ name, offset, size, modname);
+ else
+ ret = sprintf(buffer, %s+%#lx/%#lx,
+ name, offset, size);
+ }
- if (modname)
- return sprintf(buffer, %s+%#lx/%#lx [%s], name, offset,
- size, modname);
- else
- return sprintf(buffer, %s+%#lx/%#lx, name, offset, size);
+ if (namebuf)
+ kfree(namebuf);
+
+ return ret;
}
/* Look up a kernel symbol and print it to the kernel messages. */
void __print_symbol(const char *fmt, unsigned long address)
{
- char buffer[KSYM_SYMBOL_LEN];
+ char *buffer = NULL;
- sprint_symbol(buffer, address);
- printk(fmt, buffer);
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
+ if (buffer) {
+ sprint_symbol(buffer, address);
+ printk(fmt, buffer);
+ kfree(buffer);
+ } else {
+ /* Address + '0x' + NULL. */
+ char sbuffer[(BITS_PER_LONG / 4) + 3];
+
+ /* Fall-back mode.
+ Memory allocation failed.
+ Convert the address to string and display it. */
+ sprintf(sbuffer, 0x%lx, address);
+ printk(fmt, sbuffer);
+ }
}
/* To avoid using get_symbol_offset for every symbol, we carry prefix
along. */
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
Hi,
On 9/15/07, Gilboa Davara [EMAIL PROTECTED] 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 ...
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.
Gilboa Davara [EMAIL PROTECTED]
[1]
http://www.mail-archive.com/[EMAIL PROTECTED]/msg00640.html
[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.
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
--- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.0 +0300
+++ linux-2.6/kernel/kallsyms.c 2007-09-15 14:25:21.0 +0300
@@ -309,30 +309,62 @@ int lookup_symbol_attrs(unsigned long ad
/* Look up a kernel symbol and return it in a text buffer. */
int sprint_symbol(char *buffer, unsigned long address)
{
- char *modname;
- const char *name;
unsigned long offset, size;
- char namebuf[KSYM_NAME_LEN];
+ const char *name = NULL;
+ char *namebuf = NULL;
+ char *modname;
+ int ret;
+
+
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ namebuf = kmalloc(KSYM_NAME_LEN, GFP_ATOMIC);
+ if (namebuf)
+ name = kallsyms_lookup(address, size, offset,
+ modname, namebuf);
- name = kallsyms_lookup(address, size, offset, modname, namebuf);
if (!name)
- return sprintf(buffer, 0x%lx, address);
+ ret = sprintf(buffer, 0x%lx, address);
+ else {
+ if (modname)
+ ret = sprintf(buffer, %s+%#lx/%#lx [%s],
+ name, offset, size, modname);
+ else
+ ret = sprintf(buffer, %s+%#lx/%#lx,
+ name, offset, size);
+ }
- if (modname)
- return sprintf(buffer, %s+%#lx/%#lx [%s], name, offset,
- size, modname);
- else
- return sprintf(buffer, %s+%#lx/%#lx, name, offset, size);
+ if (namebuf)
+ kfree(namebuf);
+
+ return ret;
}
/* Look up a kernel symbol and print it to the kernel messages. */
void __print_symbol(const char *fmt, unsigned long address)
{
- char buffer[KSYM_SYMBOL_LEN];
+ char *buffer = NULL;
- sprint_symbol(buffer, address);
- printk(fmt, buffer);
+ /* Static buffer allocation.
+ Required in-order to reduce stack footprint on
+do_IRQ/4KSTACK/i386 */
+ buffer = kmalloc(KSYM_SYMBOL_LEN, GFP_ATOMIC);
+ if (buffer) {
+ sprint_symbol(buffer, address);
+ printk(fmt, buffer);
+ kfree(buffer);
+ } else {
+ /* Address + '0x' + NULL. */
+ char sbuffer[(BITS_PER_LONG / 4) + 3];
+
+ /* Fall-back mode.
+ Memory allocation failed.
+
Re: [Minor patch] Reduce __print_symbol/sprint_symbol stack usage.
On Sat, 2007-09-15 at 18:32 +0530, Satyam Sharma wrote: Hi, On 9/15/07, Gilboa Davara [EMAIL PROTECTED] 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
