Re: [PATCH] kallsyms data size reduction / lookup speedup

[Paulo Marques]

Matt Mackall wrote:
> On Wed, Aug 25, 2004 at 08:09:33PM +0100, Paulo Marques wrote:
>
> > diff -uprN -X dontdiff linux-2.6.9-rc1/kernel/kallsyms.c 
> > linux-2.6.9-rc1-kall/kernel/kallsyms.c
> > --- linux-2.6.9-rc1/kernel/kallsyms.c	2004-08-24 23:16:39.000000000 +0100
> > +++ linux-2.6.9-rc1-kall/kernel/kallsyms.c	2004-08-25 
> > 03:59:27.000000000 +0100
> > @@ -5,6 +5,12 @@
> >  * module loader:
> >  *   Copyright 2002 Rusty Russell <rusty@xxxxxxxxxxxxxxx> IBM Corporation
> >  * Stem compression by Andi Kleen.
>
>
> Might as well kill this dangling reference to stem compression.

Will do.

> > +static unsigned int kallsyms_expand_symbol(unsigned int off, char *result)
> > +{
> > +	int len, tlen;
> > +	u8 *tptr, *data;
> > +
> > +	data = &kallsyms_names[off];
> > +	
> > +	len=*data++;
> > +	off += len + 1;
> > +	while(len) {
> > +		tptr=&kallsyms_token_table[kallsyms_token_index[*data]];
> > +		data++;
> > +		len--;
> > +
> > +		tlen=*tptr++;
> > +		while(tlen) {
> > +			*result++=*tptr++;
>
>
> Whitespace, please. Vertical and horizontal. Also, this style with
> pointer manipulation tends to be frowned on - it's compact but
> needlessly obscure. Using a couple index variables and for loops would
> make this quite a bit more readable.

Yikes! I didn't release this code. It escaped leaving a trail of blood
in its wake :)  (Klingon programmer)

Consider this fixed, and some comments thrown in, while I'm at it.

> > +static unsigned int get_symbol_offset(unsigned long pos)
> > +{
> > +	u8 *name;
> > +	int i;
> > +
> > +	name = &kallsyms_names[ kallsyms_markers[pos>>8] ];
> > +	for(i = 0; i < (pos&0xFF); i++)
> > +		name = name + (*name) + 1;
>
>
> That's a bit magic looking. A brief description of the data structure in this
> vicinity would be appreciated. 

Ok.

> > --- linux-2.6.9-rc1/scripts/kallsyms.c	2004-08-24 23:16:39.000000000 +0100
> > +++ linux-2.6.9-rc1-kall/scripts/kallsyms.c	2004-08-25 
> > 03:30:30.000000000 +0100
> > @@ -6,6 +6,13 @@
> >  * of the GNU General Public License, incorporated herein by reference.
> >  *
> >  * Usage: nm -n vmlinux | scripts/kallsyms [--all-symbols] > symbols.S
> > + *
> > + * ChangeLog:
> > + *
> > + * (25/Aug/2004) Paulo Marques
> > + *      Changed the compression method from stem compression to "table 
> > lookup"
> > + *      compression
> > + *
> >  */
>
>
> Throw an address in there so we know where to direct complaints. Also
> a description of "table compression" belongs here.

Ok.

> Generally looks decent, could use a bit more in the way of comments.

The next patch will come with all this fixed. I was actually afraid of
putting in too many comments, that I ended up putting hardly any
comments.

Thanks for all the sugestions,

--
Paulo Marques - www.grupopie.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/