Re: [PATCH] Reduce __print_symbol/sprint_symbol stack usage.

[Gilboa Davara <gilboad@gmail.com>]

Hello Satyam,

On Wed, 2007-09-19 at 06:30 +0530, Satyam Sharma wrote: 
> 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 ...

ACK.
Will be fixed ASAP.

> 
> > +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 ?

I did my best to keep the old interface (sprint_symbol), char arrays and
all, fixing only the critical dump_stack path. (Hence, print_symbol no
longer calls sprint_symbol - it calls the __sprint_symbol helper
function instead.

> 
> > +
> > +	return __sprint_symbol(buffer, namebuf, address);
> 
> And you'd need to wrap spin_lock_irqsave()/spin_unlock_irqrestore()
> around this call.

As I'm keeping the old interface (with it's static char arrays) for
non-dump_stack paths, there's no need to use locks around sprint_symbol.

> 
> > +}
> 
> 
> > +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 ...

ACK. 

> 
> 
> >  /* 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);

ACK.

> 
> 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 :-(

Yep. Still needs work.
... I'll get there. Dead or alive. ;)

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

The -best- solution is to use locking in normal circumstances (E.g.
debug code, sleep while atomic ,etc) -but- drop all locks when BUG/Oops
is being hit.

I believe the next patch will take care of this problem.

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

Any suggestions?

- Gilboa