Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752481AbYHZUOf (ORCPT ); Tue, 26 Aug 2008 16:14:35 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751036AbYHZUOW (ORCPT ); Tue, 26 Aug 2008 16:14:22 -0400 Received: from mga03.intel.com ([143.182.124.21]:38501 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbYHZUOU (ORCPT ); Tue, 26 Aug 2008 16:14:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.32,273,1217833200"; d="scan'208";a="37815312" Message-ID: <48B4641A.1020806@intel.com> Date: Tue, 26 Aug 2008 13:14:18 -0700 From: "Kok, Auke" User-Agent: Thunderbird 2.0.0.14 (X11/20080626) MIME-Version: 1.0 To: Linus Torvalds , Jeff Kirsher CC: Jeff Garzik , Rusty Russell , "Alan D. Brunelle" , "Rafael J. Wysocki" , Linux Kernel Mailing List , Kernel Testers List , Andrew Morton , Arjan van de Ven , Ingo Molnar Subject: Re: e1000 horridness (was Re: [Bug #11342] Linux 2.6.27-rc3: kernel BUG at mm/vmalloc.c - bisected) References: <48B313E0.1000501@hp.com> <200808261111.19205.rusty@rustcorp.com.au> <48B45FA2.8040603@garzik.org> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 26 Aug 2008 20:14:19.0549 (UTC) FILETIME=[527424D0:01C907B8] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10763 Lines: 308 Linus Torvalds wrote: > > On Tue, 26 Aug 2008, Jeff Garzik wrote: >> e1000_check_options builds a struct (singular) on the stack, really... struct >> e1000_option is reasonably small. > > No it doesn't. > > Look a bit more closely. > > It builds a struct (singular) MANY MANY times. It also then builds up a > huge e1000_opt_list[] array, even though it is const and should be static > (and const). > > I know. I wrote a patch to FIX it. totally cool patch afaics - if I still maintained this driver I'd have this tested and merged right away :) I suppose Jeff Kirsher is already doing so right now. I suppose that he'll have to look at the other Intel ethernet drivers as well :) Jeff, please add my: Reveiewed-by: Auke Kok Cheers, Auke > > Here's the patch. It shrinks the stack from 1152 bytes to 192 bytes (the > first version, that only did the e1000_option part, got it down to 600 > bytes). About half comes from not using multiple "e1000_option" > structures, the other half comes from turning the "e1000_opt_list[]" > arrays into "static const" instead, so that gcc doesn't copy them onto the > stack. > > Most of the patch is actually doing things like turning > > struct struct e1000_option opt = { > > (which declares a _new_ e1000_option variable each time) into > > opt = (struct e1000_option) { > > which just re-uses the single variable. > > It becomes slightly larger than that, because some places the "opt = .." > had to be moved around, since it's no longer a variable declaration, but a > regular assignment. > > The rest is just adding "const" to the right places, and turning > > struct e1000_opt_list speed_list[] = .. > > into > > static const struct e1000_opt_list speed_list[] = .. > > instead, and fixing the indentation to be more straightforward. > > I have not tested the dang thing, but I think it's correct. And it turns > stack usage from "totally horrible and broken" into "pretty reasonable". > > Linus > > --- > drivers/net/e1000/e1000_param.c | 81 +++++++++++++++++++++----------------- > 1 files changed, 45 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/e1000/e1000_param.c b/drivers/net/e1000/e1000_param.c > index b9f90a5..213437d 100644 > --- a/drivers/net/e1000/e1000_param.c > +++ b/drivers/net/e1000/e1000_param.c > @@ -208,7 +208,7 @@ struct e1000_option { > } r; > struct { /* list_option info */ > int nr; > - struct e1000_opt_list { int i; char *str; } *p; > + const struct e1000_opt_list { int i; char *str; } *p; > } l; > } arg; > }; > @@ -242,7 +242,7 @@ static int __devinit e1000_validate_option(unsigned int *value, > break; > case list_option: { > int i; > - struct e1000_opt_list *ent; > + const struct e1000_opt_list *ent; > > for (i = 0; i < opt->arg.l.nr; i++) { > ent = &opt->arg.l.p[i]; > @@ -279,7 +279,9 @@ static void e1000_check_copper_options(struct e1000_adapter *adapter); > > void __devinit e1000_check_options(struct e1000_adapter *adapter) > { > + struct e1000_option opt; > int bd = adapter->bd_number; > + > if (bd >= E1000_MAX_NIC) { > DPRINTK(PROBE, NOTICE, > "Warning: no configuration for board #%i\n", bd); > @@ -287,19 +289,21 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > > { /* Transmit Descriptor Count */ > - struct e1000_option opt = { > + struct e1000_tx_ring *tx_ring = adapter->tx_ring; > + int i; > + e1000_mac_type mac_type = adapter->hw.mac_type; > + > + opt = (struct e1000_option) { > .type = range_option, > .name = "Transmit Descriptors", > .err = "using default of " > __MODULE_STRING(E1000_DEFAULT_TXD), > .def = E1000_DEFAULT_TXD, > - .arg = { .r = { .min = E1000_MIN_TXD }} > + .arg = { .r = { > + .min = E1000_MIN_TXD, > + .max = mac_type < e1000_82544 ? E1000_MAX_TXD : E1000_MAX_82544_TXD > + }} > }; > - struct e1000_tx_ring *tx_ring = adapter->tx_ring; > - int i; > - e1000_mac_type mac_type = adapter->hw.mac_type; > - opt.arg.r.max = mac_type < e1000_82544 ? > - E1000_MAX_TXD : E1000_MAX_82544_TXD; > > if (num_TxDescriptors > bd) { > tx_ring->count = TxDescriptors[bd]; > @@ -313,19 +317,21 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > tx_ring[i].count = tx_ring->count; > } > { /* Receive Descriptor Count */ > - struct e1000_option opt = { > + struct e1000_rx_ring *rx_ring = adapter->rx_ring; > + int i; > + e1000_mac_type mac_type = adapter->hw.mac_type; > + > + opt = (struct e1000_option) { > .type = range_option, > .name = "Receive Descriptors", > .err = "using default of " > __MODULE_STRING(E1000_DEFAULT_RXD), > .def = E1000_DEFAULT_RXD, > - .arg = { .r = { .min = E1000_MIN_RXD }} > + .arg = { .r = { > + .min = E1000_MIN_RXD, > + .max = mac_type < e1000_82544 ? E1000_MAX_RXD : E1000_MAX_82544_RXD > + }} > }; > - struct e1000_rx_ring *rx_ring = adapter->rx_ring; > - int i; > - e1000_mac_type mac_type = adapter->hw.mac_type; > - opt.arg.r.max = mac_type < e1000_82544 ? E1000_MAX_RXD : > - E1000_MAX_82544_RXD; > > if (num_RxDescriptors > bd) { > rx_ring->count = RxDescriptors[bd]; > @@ -339,7 +345,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > rx_ring[i].count = rx_ring->count; > } > { /* Checksum Offload Enable/Disable */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = enable_option, > .name = "Checksum Offload", > .err = "defaulting to Enabled", > @@ -363,7 +369,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > { E1000_FC_FULL, "Flow Control Enabled" }, > { E1000_FC_DEFAULT, "Flow Control Hardware Default" }}; > > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = list_option, > .name = "Flow Control", > .err = "reading default settings from EEPROM", > @@ -381,7 +387,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Transmit Interrupt Delay */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = range_option, > .name = "Transmit Interrupt Delay", > .err = "using default of " __MODULE_STRING(DEFAULT_TIDV), > @@ -399,7 +405,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Transmit Absolute Interrupt Delay */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = range_option, > .name = "Transmit Absolute Interrupt Delay", > .err = "using default of " __MODULE_STRING(DEFAULT_TADV), > @@ -417,7 +423,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Receive Interrupt Delay */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = range_option, > .name = "Receive Interrupt Delay", > .err = "using default of " __MODULE_STRING(DEFAULT_RDTR), > @@ -435,7 +441,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Receive Absolute Interrupt Delay */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = range_option, > .name = "Receive Absolute Interrupt Delay", > .err = "using default of " __MODULE_STRING(DEFAULT_RADV), > @@ -453,7 +459,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Interrupt Throttling Rate */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = range_option, > .name = "Interrupt Throttling Rate (ints/sec)", > .err = "using default of " __MODULE_STRING(DEFAULT_ITR), > @@ -497,7 +503,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Smart Power Down */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = enable_option, > .name = "PHY Smart Power Down", > .err = "defaulting to Disabled", > @@ -513,7 +519,7 @@ void __devinit e1000_check_options(struct e1000_adapter *adapter) > } > } > { /* Kumeran Lock Loss Workaround */ > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = enable_option, > .name = "Kumeran Lock Loss Workaround", > .err = "defaulting to Enabled", > @@ -578,16 +584,18 @@ static void __devinit e1000_check_fiber_options(struct e1000_adapter *adapter) > > static void __devinit e1000_check_copper_options(struct e1000_adapter *adapter) > { > + struct e1000_option opt; > unsigned int speed, dplx, an; > int bd = adapter->bd_number; > > { /* Speed */ > - struct e1000_opt_list speed_list[] = {{ 0, "" }, > - { SPEED_10, "" }, > - { SPEED_100, "" }, > - { SPEED_1000, "" }}; > + static const struct e1000_opt_list speed_list[] = { > + { 0, "" }, > + { SPEED_10, "" }, > + { SPEED_100, "" }, > + { SPEED_1000, "" }}; > > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = list_option, > .name = "Speed", > .err = "parameter ignored", > @@ -604,11 +612,12 @@ static void __devinit e1000_check_copper_options(struct e1000_adapter *adapter) > } > } > { /* Duplex */ > - struct e1000_opt_list dplx_list[] = {{ 0, "" }, > - { HALF_DUPLEX, "" }, > - { FULL_DUPLEX, "" }}; > + static const struct e1000_opt_list dplx_list[] = { > + { 0, "" }, > + { HALF_DUPLEX, "" }, > + { FULL_DUPLEX, "" }}; > > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = list_option, > .name = "Duplex", > .err = "parameter ignored", > @@ -637,7 +646,7 @@ static void __devinit e1000_check_copper_options(struct e1000_adapter *adapter) > "parameter ignored\n"); > adapter->hw.autoneg_advertised = AUTONEG_ADV_DEFAULT; > } else { /* Autoneg */ > - struct e1000_opt_list an_list[] = > + static const struct e1000_opt_list an_list[] = > #define AA "AutoNeg advertising " > {{ 0x01, AA "10/HD" }, > { 0x02, AA "10/FD" }, > @@ -671,7 +680,7 @@ static void __devinit e1000_check_copper_options(struct e1000_adapter *adapter) > { 0x2e, AA "1000/FD, 100/FD, 100/HD, 10/FD" }, > { 0x2f, AA "1000/FD, 100/FD, 100/HD, 10/FD, 10/HD" }}; > > - struct e1000_option opt = { > + opt = (struct e1000_option) { > .type = list_option, > .name = "AutoNeg", > .err = "parameter ignored", -- 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/