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

[Satyam Sharma <satyam@infradead.org>]

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/