lkml.org 
[lkml]   [2007]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.
Hi Gilboa,


On Sat, 15 Sep 2007, Gilboa Davara wrote:
>
> This is my second stab at solving the "stack over flow due to
> dump_strace when close to stack-overflow is detected by do_IRQ" problem.
> (Hopefully) this patch is creates less noise then the previous one.
>
> [snip]
> > 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
>
> --- linux-2.6/kernel/kallsyms.orig 2007-09-15 11:46:54.000000000 +0300
> +++ linux-2.6/kernel/kallsyms.c 2007-09-15 21:06:55.000000000 +0300
> @@ -306,13 +306,14 @@ int lookup_symbol_attrs(unsigned long ad
> return lookup_module_symbol_attrs(addr, size, offset, modname, name);
> }
>
> -/* Look up a kernel symbol and return it in a text buffer. */
> -int sprint_symbol(char *buffer, unsigned long address)
> +/* Internal version:
> + Look up a kernel symbol and module name and return them to the
> + caller's buffer/namebuf buffers. */

/*
* ...
* ...
*/
is the general coding style here ...

> +int __sprint_symbol(char *buffer, char *namebuf, unsigned long address)
> {
> - char *modname;
> - const char *name;
> unsigned long offset, size;
> - char namebuf[KSYM_NAME_LEN];
> + const char *name;
> + char *modname;
>
> name = kallsyms_lookup(address, &size, &offset, &modname, namebuf);
> if (!name)
> @@ -325,14 +326,35 @@ int sprint_symbol(char *buffer, unsigned
> return sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
> }
>
> +/* Exported version:
> + Look up a kernel symbol and return it in a text buffer. */

ditto.

> +int sprint_symbol(char *buffer, unsigned long address)
> +{
> + char namebuf[KSYM_NAME_LEN];

Hmm, don't we intend to push this array out of the stack too?

+ static char namebuf[KSYM_NAME_LEN];
+ static DEFINE_SPINLOCK(namebuf_lock);

here ?

> +
> + return __sprint_symbol(buffer, namebuf, address);

And you'd need to wrap spin_lock_irqsave()/spin_unlock_irqrestore()
around this call.

> +}


> +static DEFINE_SPINLOCK(symbol_lock);

Try to keep the declarations of a lock, and the data that it protects,
close together. Since this lock is being used to protect "buffer", it
makes sense to ...


> /* 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];
> + /* Use static buffers instead of char array to reduce
> + stack footprint in i386/4KSTACKS.
> + Buffers must be protected against re-entry. */
> + static char namebuf[KSYM_NAME_LEN];
> + static char buffer[KSYM_SYMBOL_LEN];

... have it:

+ static DEFINE_SPINLOCK(buffer_lock);

here (note the name that exactly describes what the lock protects).

And the namebuf array isn't required here, it's already there in
sprint_symbol(), which you can call from ...

> + unsigned long flags;
> +
>
> - sprint_symbol(buffer, address);
> + spin_lock_irqsave(&symbol_lock, flags);
> +
> + __sprint_symbol(buffer, namebuf, address);

here ... sprint_symbol() ?

> printk(fmt, buffer);
> +
> + spin_unlock_irqrestore(&symbol_lock, flags);

But I still don't much like this :-(

More importantly, if a panic occurs *below* this callchain (and let's
say we ended up in this callchain because somebody put in a dump_stack()
somewhere for debugging purposes), then we'd have a deadlock on our hands,
and nothing gets printed for that panic.

I don't know who maintains this part of kernel code, but you can try
resubmitting (with the changes suggested above) to someone appropriate ...


Satyam
-
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-19 02:59    [from the cache]
©2003-2011 Jasper Spaans