* [PATCH] reduce stack usage in ixgb_ethtool_ioctl
@ 2004-09-19 17:33 Denis Vlasenko
2004-09-19 18:24 ` Dave Dillow
0 siblings, 1 reply; 4+ messages in thread
From: Denis Vlasenko @ 2004-09-19 17:33 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-kernel, netdev
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]
Stack usage reduced from 3024 (!) to 576.
Most of those 3k came from a bug:
#define IXGB_REG_DUMP_LEN 136*sizeof(uint32_t)
^^^^^^^^^^^^^^^^^
...
uint32_t regs_buff[IXGB_REG_DUMP_LEN];
Bug fixed. Two large on-stack variables moved to kmalloc space.
Stack usage is still high because gcc will
allocate too much space for these cases:
case ETHTOOL_GSET:{
struct ethtool_cmd ecmd = { ETHTOOL_GSET };
ixgb_ethtool_gset(adapter, &ecmd);
if (copy_to_user(addr, &ecmd, sizeof(ecmd)))
return -EFAULT;
return 0;
}
case ETHTOOL_SSET:{
struct ethtool_cmd ecmd;
if (copy_from_user(&ecmd, addr, sizeof(ecmd)))
return -EFAULT;
return ixgb_ethtool_sset(adapter, &ecmd);
}
There will be space for _two_ ecmd's on stack.
Shall it be worked around with ugly union of structs
or we'll just wait for better gcc?
Compile-tested only.
--
vda
[-- Attachment #2: linux-2.6.9-rc2.ixgb_ethtool.patch --]
[-- Type: text/x-diff, Size: 3613 bytes --]
diff -urp linux-2.6.9-rc2.src/drivers/net/ixgb/ixgb_ethtool.c linux-2.6.9-rc2.ldelf/drivers/net/ixgb/ixgb_ethtool.c
--- linux-2.6.9-rc2.src/drivers/net/ixgb/ixgb_ethtool.c Sat Aug 14 13:56:09 2004
+++ linux-2.6.9-rc2.ldelf/drivers/net/ixgb/ixgb_ethtool.c Sun Sep 19 20:20:12 2004
@@ -180,8 +180,8 @@ ixgb_ethtool_gdrvinfo(struct ixgb_adapte
strncpy(drvinfo->fw_version, "N/A", 32);
strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
drvinfo->n_stats = IXGB_STATS_LEN;
-#define IXGB_REG_DUMP_LEN 136*sizeof(uint32_t)
- drvinfo->regdump_len = IXGB_REG_DUMP_LEN;
+#define IXGB_REG_DUMP_CNT 136
+ drvinfo->regdump_len = IXGB_REG_DUMP_CNT*sizeof(uint32_t);
drvinfo->eedump_len = ixgb_eeprom_size(&adapter->hw);
}
@@ -459,6 +459,7 @@ int ixgb_ethtool_ioctl(struct net_device
struct ixgb_adapter *adapter = netdev->priv;
void __user *addr = ifr->ifr_data;
uint32_t cmd;
+ int err;
if (get_user(cmd, (uint32_t __user *) addr))
return -EFAULT;
@@ -487,8 +488,8 @@ int ixgb_ethtool_ioctl(struct net_device
case ETHTOOL_GSTRINGS:{
struct ethtool_gstrings gstrings = { ETHTOOL_GSTRINGS };
char *strings = NULL;
- int err = 0;
+ err = 0;
if (copy_from_user(&gstrings, addr, sizeof(gstrings)))
return -EFAULT;
switch (gstrings.string_set) {
@@ -526,19 +527,28 @@ int ixgb_ethtool_ioctl(struct net_device
}
case ETHTOOL_GREGS:{
struct ethtool_regs regs = { ETHTOOL_GREGS };
- uint32_t regs_buff[IXGB_REG_DUMP_LEN];
+ uint32_t *regs_buff;
+ err = -ENOMEM;
+ regs_buff = kmalloc(sizeof(*regs_buff)*IXGB_REG_DUMP_CNT, GFP_KERNEL);
+ if (!regs_buff)
+ goto ret_err;
+
+ err = -EFAULT;
if (copy_from_user(®s, addr, sizeof(regs)))
- return -EFAULT;
+ goto ret_err;
ixgb_ethtool_gregs(adapter, ®s, regs_buff);
if (copy_to_user(addr, ®s, sizeof(regs)))
- return -EFAULT;
+ goto ret_err;
addr += offsetof(struct ethtool_regs, data);
if (copy_to_user(addr, regs_buff, regs.len))
- return -EFAULT;
+ goto ret_err;
+ err = 0;
- return 0;
+ ret_err:
+ kfree(regs_buff);
+ return err;
}
case ETHTOOL_NWAY_RST:{
if (netif_running(netdev)) {
@@ -565,8 +575,8 @@ int ixgb_ethtool_ioctl(struct net_device
struct ethtool_eeprom eeprom = { ETHTOOL_GEEPROM };
uint16_t eeprom_buff[IXGB_EEPROM_SIZE];
void *ptr;
- int err = 0;
+ err = 0;
if (copy_from_user(&eeprom, addr, sizeof(eeprom)))
return -EFAULT;
@@ -609,15 +619,20 @@ int ixgb_ethtool_ioctl(struct net_device
return ixgb_ethtool_spause(adapter, &epause);
}
case ETHTOOL_GSTATS:{
+ int i;
struct {
struct ethtool_stats eth_stats;
uint64_t data[IXGB_STATS_LEN];
- } stats = { {
- ETHTOOL_GSTATS, IXGB_STATS_LEN}};
- int i;
+ } *stats;
+
+ stats = kmalloc(sizeof(*stats), GFP_KERNEL);
+ if (!stats)
+ return -ENOMEM;
+ stats->eth_stats.cmd = ETHTOOL_GSTATS;
+ stats->eth_stats.n_stats = IXGB_STATS_LEN;
for (i = 0; i < IXGB_STATS_LEN; i++)
- stats.data[i] =
+ stats->data[i] =
(ixgb_gstrings_stats[i].sizeof_stat ==
sizeof(uint64_t)) ? *(uint64_t *) ((char *)
adapter
@@ -628,9 +643,11 @@ int ixgb_ethtool_ioctl(struct net_device
: *(uint32_t *) ((char *)adapter +
ixgb_gstrings_stats[i].
stat_offset);
- if (copy_to_user(addr, &stats, sizeof(stats)))
- return -EFAULT;
- return 0;
+ err = 0;
+ if (copy_to_user(addr, stats, sizeof(*stats)))
+ err = -EFAULT;
+ kfree(stats);
+ return err;
}
case ETHTOOL_GRXCSUM:{
struct ethtool_value edata = { ETHTOOL_GRXCSUM };
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] reduce stack usage in ixgb_ethtool_ioctl
2004-09-19 17:33 [PATCH] reduce stack usage in ixgb_ethtool_ioctl Denis Vlasenko
@ 2004-09-19 18:24 ` Dave Dillow
2004-09-19 18:26 ` Jeff Garzik
0 siblings, 1 reply; 4+ messages in thread
From: Dave Dillow @ 2004-09-19 18:24 UTC (permalink / raw)
To: Denis Vlasenko; +Cc: Jeff Garzik, linux-kernel, netdev
On Sun, 2004-09-19 at 13:33, Denis Vlasenko wrote:
> Stack usage is still high because gcc will
> allocate too much space for these cases:
>
> case ETHTOOL_GSET:{
> struct ethtool_cmd ecmd = { ETHTOOL_GSET };
> ixgb_ethtool_gset(adapter, &ecmd);
> if (copy_to_user(addr, &ecmd, sizeof(ecmd)))
> return -EFAULT;
> return 0;
> }
> case ETHTOOL_SSET:{
> struct ethtool_cmd ecmd;
> if (copy_from_user(&ecmd, addr, sizeof(ecmd)))
> return -EFAULT;
> return ixgb_ethtool_sset(adapter, &ecmd);
> }
>
> There will be space for _two_ ecmd's on stack.
>
> Shall it be worked around with ugly union of structs
> or we'll just wait for better gcc?
You could convert it to use ethtool_ops.
--
Dave Dillow <dave@thedillows.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] reduce stack usage in ixgb_ethtool_ioctl
2004-09-19 18:24 ` Dave Dillow
@ 2004-09-19 18:26 ` Jeff Garzik
2004-09-19 18:47 ` Denis Vlasenko
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Garzik @ 2004-09-19 18:26 UTC (permalink / raw)
To: Dave Dillow; +Cc: Denis Vlasenko, linux-kernel, netdev
Dave Dillow wrote:
> On Sun, 2004-09-19 at 13:33, Denis Vlasenko wrote:
>
>>Stack usage is still high because gcc will
>>allocate too much space for these cases:
>>
>> case ETHTOOL_GSET:{
>> struct ethtool_cmd ecmd = { ETHTOOL_GSET };
>> ixgb_ethtool_gset(adapter, &ecmd);
>> if (copy_to_user(addr, &ecmd, sizeof(ecmd)))
>> return -EFAULT;
>> return 0;
>> }
>> case ETHTOOL_SSET:{
>> struct ethtool_cmd ecmd;
>> if (copy_from_user(&ecmd, addr, sizeof(ecmd)))
>> return -EFAULT;
>> return ixgb_ethtool_sset(adapter, &ecmd);
>> }
>>
>>There will be space for _two_ ecmd's on stack.
>>
>>Shall it be worked around with ugly union of structs
>>or we'll just wait for better gcc?
>
>
> You could convert it to use ethtool_ops.
Check -mm to make sure viro hasn't already converted it to ethtool_ops...
Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] reduce stack usage in ixgb_ethtool_ioctl
2004-09-19 18:26 ` Jeff Garzik
@ 2004-09-19 18:47 ` Denis Vlasenko
0 siblings, 0 replies; 4+ messages in thread
From: Denis Vlasenko @ 2004-09-19 18:47 UTC (permalink / raw)
To: Jeff Garzik, Dave Dillow; +Cc: linux-kernel, netdev
> >> case ETHTOOL_GSET:{
> >> struct ethtool_cmd ecmd = { ETHTOOL_GSET };
> >> ixgb_ethtool_gset(adapter, &ecmd);
> >> if (copy_to_user(addr, &ecmd, sizeof(ecmd)))
> >> return -EFAULT;
> >> return 0;
> >> }
> >> case ETHTOOL_SSET:{
> >> struct ethtool_cmd ecmd;
> >> if (copy_from_user(&ecmd, addr, sizeof(ecmd)))
> >> return -EFAULT;
> >> return ixgb_ethtool_sset(adapter, &ecmd);
> >> }
> >>
> >>There will be space for _two_ ecmd's on stack.
> >>
> >>Shall it be worked around with ugly union of structs
> >>or we'll just wait for better gcc?
> >
> > You could convert it to use ethtool_ops.
>
> Check -mm to make sure viro hasn't already converted it to ethtool_ops...
Admit it: it's a conspiracy. Whenever I take some code to hack on, somebody
else takes care of it before I do ;)
--
vda
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-09-19 18:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-09-19 17:33 [PATCH] reduce stack usage in ixgb_ethtool_ioctl Denis Vlasenko
2004-09-19 18:24 ` Dave Dillow
2004-09-19 18:26 ` Jeff Garzik
2004-09-19 18:47 ` Denis Vlasenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).