Re: [PATCH linux-2.6.12-rc2-mm3] smc91c92_cs: Reduce stack usage in smc91c92_event()
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 = _mem->tuple;
> + parse = _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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH linux-2.6.12-rc2-mm3] smc91c92_cs: Reduce stack usage in smc91c92_event()
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH linux-2.6.12-rc2-mm3] smc91c92_cs: Reduce stack usage in smc91c92_event()
On Friday 22 April 2005 01:02, Yum Rayan wrote:
> This patch reduces the stack usage of the function smc91c92_event() in
> smc91c92_cs driver from 3540 to 132. Currently this is the highest
> stack user in linux-2.6.12-rc2-mm3. I used a patched version of gcc
> 3.4.3 on i386 with -fno-unit-at-a-time disabled.
>
> The patch has only been compile tested. It would be nice to get
> feedback if someone that owns the hardware can actually test this
> patch.
>
> Acked-by: JЖrn Engel <[EMAIL PROTECTED]>
> Acked-by: Randy Dunlap <[EMAIL PROTECTED]>
> Signed-off-by: Yum Rayan <[EMAIL PROTECTED]>
>
> smc91c92_cs.c | 287
> ++
> 1 files changed, 189 insertions(+), 98 deletions(-)
>
> diff -Nupr -X dontdiff
> linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c
> linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c
> --- linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c 2005-04-14
> 22:15:43.0 -0700
> +++ linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c 2005-04-20
> 18:12:00.0 -0700
> @@ -127,6 +127,12 @@ struct smc_private {
> int rx_ovrn;
> };
>
> +struct smc_cfg_mem {
> +tuple_t tuple;
> +cisparse_t parse;
> +u_char buf[255];
> +};
> +
> /* Special definitions for Megahertz multifunction cards */
> #define MEGAHERTZ_ISR0x0380
>
> @@ -498,14 +504,24 @@ static int mhz_mfc_config(dev_link_t *li
> {
> struct net_device *dev = link->priv;
> struct smc_private *smc = netdev_priv(dev);
> -tuple_t tuple;
> -cisparse_t parse;
> -u_char buf[255];
> -cistpl_cftable_entry_t *cf = _entry;
> +struct smc_cfg_mem *cfg_mem;
> +tuple_t *tuple;
> +cisparse_t *parse;
> +cistpl_cftable_entry_t *cf;
> +u_char *buf;
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.
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 = _mem->tuple;
+ parse = _mem->parse;
+ buf = cfg_mem->buf;
4) in resulting asm one base pointer instead of many will require
less registers
Look into nfs4_proc_unlink_setup() for example.
I see that Trond do not use locally declared struct there,
but otherwise it is done as described above.
--
vda
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH linux-2.6.12-rc2-mm3] smc91c92_cs: Reduce stack usage in smc91c92_event()
On Friday 22 April 2005 01:02, Yum Rayan wrote:
This patch reduces the stack usage of the function smc91c92_event() in
smc91c92_cs driver from 3540 to 132. Currently this is the highest
stack user in linux-2.6.12-rc2-mm3. I used a patched version of gcc
3.4.3 on i386 with -fno-unit-at-a-time disabled.
The patch has only been compile tested. It would be nice to get
feedback if someone that owns the hardware can actually test this
patch.
Acked-by: Jrn Engel [EMAIL PROTECTED]
Acked-by: Randy Dunlap [EMAIL PROTECTED]
Signed-off-by: Yum Rayan [EMAIL PROTECTED]
smc91c92_cs.c | 287
++
1 files changed, 189 insertions(+), 98 deletions(-)
diff -Nupr -X dontdiff
linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c
linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c
--- linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c 2005-04-14
22:15:43.0 -0700
+++ linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c 2005-04-20
18:12:00.0 -0700
@@ -127,6 +127,12 @@ struct smc_private {
int rx_ovrn;
};
+struct smc_cfg_mem {
+tuple_t tuple;
+cisparse_t parse;
+u_char buf[255];
+};
+
/* Special definitions for Megahertz multifunction cards */
#define MEGAHERTZ_ISR0x0380
@@ -498,14 +504,24 @@ static int mhz_mfc_config(dev_link_t *li
{
struct net_device *dev = link-priv;
struct smc_private *smc = netdev_priv(dev);
-tuple_t tuple;
-cisparse_t parse;
-u_char buf[255];
-cistpl_cftable_entry_t *cf = parse.cftable_entry;
+struct smc_cfg_mem *cfg_mem;
+tuple_t *tuple;
+cisparse_t *parse;
+cistpl_cftable_entry_t *cf;
+u_char *buf;
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.
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
Look into nfs4_proc_unlink_setup() for example.
I see that Trond do not use locally declared struct there,
but otherwise it is done as described above.
--
vda
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
