lkml.org 
[lkml]   [2007]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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    [W:0.027 / U:358.544 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site