lkml.org 
[lkml]   [2008]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: e1000 horridness (was Re: [Bug #11342] Linux 2.6.27-rc3: kernel BUG at mm/vmalloc.c - bisected)
    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 <auke-jan.h.kok@intel.com>

    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",



    \
     
     \ /
      Last update: 2008-08-26 22:17    [W:7.258 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site