Re: [PATCH linux-2.6.12-rc2-mm3] smc91c92_cs: Reduce stack usage in smc91c92_event()

["Jörn Engel" <joern@wohnheim.fh-wedel.de>]

On Fri, 22 April 2005 11:22:51 +0300, Denis Vlasenko wrote:
> 
> I do it this way:
> 
> int f()
> {
> -	tuple_t tuple;
> -	cisparse_t parse;
> -	u_char buf[255];
> +	struct {
> +		tuple_t tuple;
> +		cisparse_t parse;
> +		u_char buf[255];
> +	} local;
> +	local = kmalloc(sizeof(*local),...); if(!local)...
> 	...
> -    	tuple.Attributes = tuple.TupleOffset = 0;
> -    	tuple.TupleData = (cisdata_t *)buf;
> -    	tuple.TupleDataMax = sizeof(buf);
> +    	local->tuple.Attributes = local->tuple.TupleOffset = 0;
> +    	local->tuple.TupleData = (cisdata_t *)local->buf;
> +    	local->tuple.TupleDataMax = sizeof(local->buf);
> 
> I see the following advantages:
> 
> 1) struct is unnamed and local to function
> 2) Variables do not change their type, the just sit in local-> now.
>    I can just add 'local->' to each affected variable,
>    without "it was an object, now it is a pointer" changes.
>    No need to replace . with ->, remove &, etc.

I'd have proposed the same, before reading further down in the patch.
Basically, the driver is full of duplication, so the exact same struct
can be used several times.  Therefore, the downsides of your approach
seem to prevail.

> 3) I do not need to do this part of your patch which adds more locals:
> +    tuple_t *tuple;
> +    cisparse_t *parse;
> +    cistpl_cftable_entry_t *cf;
> +    u_char *buf;
> ...
> +    tuple = &cfg_mem->tuple;
> +    parse = &cfg_mem->parse;
> +    buf = cfg_mem->buf;
> 4) in resulting asm one base pointer instead of many will require
>    less registers

Yup.  There are thousands of detail to improve in that driver.  It's
current maintainership (there is none) may explain that state.

Jörn

-- 
Fantasy is more important than knowledge. Knowledge is limited,
while fantasy embraces the whole world.
-- Albert Einstein