Message ID | 12958.1274145714@death.nxdomain.ibm.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, May 17, 2010 at 9:21 PM, Jay Vosburgh <fubar@us.ibm.com> wrote: > Jay Vosburgh <fubar@us.ibm.com> wrote: > [...] >> For your patch, I'm exploring the idea of not setting >>IFF_SLAVE_INACTIVE on "inactive" slaves for an "all_slaves_active" >>option (I think that's a more descriptive name than "keep_all") instead >>of adding a new KEEP_ALL flag bit to priv_flags. Did you consider this >>methodology and exclude it for some reason? > > Following up to myself, I coded up approximately what I was > talking about. This doesn't require the extra priv_flag, and the sysfs > _store is a little more complicated, but this appears to work (testing > with ping -f after clearing the switch's MAC table to induce traffic > flooding). I didn't change the option name from "keep_all" here, but as > far as the functionality goes, this seems to do what I think you want it > to. > I can test it here to be sure, but overall this seems like a fine approach. The IFF_SLAVE_INACTIVE doesn't seem to be used for much other than duplicate suppression at this point, so it seems like a logical choice. As for the original name 'keep_all,' I tried to use something that user would find easy to understand. Obviously not all users think alike. :-) > -J > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 5e12462..c80d2ff 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -106,6 +106,7 @@ static int arp_interval = BOND_LINK_ARP_INTERV; > static char *arp_ip_target[BOND_MAX_ARP_TARGETS]; > static char *arp_validate; > static char *fail_over_mac; > +static int keep_all = 0; > static struct bond_params bonding_defaults; > > module_param(max_bonds, int, 0); > @@ -155,6 +156,9 @@ module_param(arp_validate, charp, 0); > MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes: none (default), active, backup or all"); > module_param(fail_over_mac, charp, 0); > MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to the same MAC. none (default), active or follow"); > +module_param(keep_all, int, 0); > +MODULE_PARM_DESC(keep_all, "Keep all frames received on an interface" > + "0 for never (default), 1 for always."); > > /*----------------------------- Global variables ----------------------------*/ > > @@ -4756,6 +4760,13 @@ static int bond_check_params(struct bond_params *params) > } > } > > + if ((keep_all != 0) && (keep_all != 1)) { > + pr_warning("Warning: keep_all module parameter (%d), " > + "not of valid value (0/1), so it was set to " > + "0\n", keep_all); > + keep_all = 0; > + } > + > /* reset values for TLB/ALB */ > if ((bond_mode == BOND_MODE_TLB) || > (bond_mode == BOND_MODE_ALB)) { > @@ -4926,6 +4937,7 @@ static int bond_check_params(struct bond_params *params) > params->primary[0] = 0; > params->primary_reselect = primary_reselect_value; > params->fail_over_mac = fail_over_mac_value; > + params->keep_all = keep_all; > > if (primary) { > strncpy(params->primary, primary, IFNAMSIZ); > diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c > index b8bec08..73b3c03 100644 > --- a/drivers/net/bonding/bond_sysfs.c > +++ b/drivers/net/bonding/bond_sysfs.c > @@ -339,7 +339,6 @@ out: > > static DEVICE_ATTR(slaves, S_IRUGO | S_IWUSR, bonding_show_slaves, > bonding_store_slaves); > - > /* > * Show and set the bonding mode. The bond interface must be down to > * change the mode. > @@ -1472,6 +1471,59 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d, > } > static DEVICE_ATTR(ad_partner_mac, S_IRUGO, bonding_show_ad_partner_mac, NULL); > > +/* > + * Show and set the keep_all flag. > + */ > +static ssize_t bonding_show_keep(struct device *d, > + struct device_attribute *attr, > + char *buf) > +{ > + struct bonding *bond = to_bond(d); > + > + return sprintf(buf, "%d\n", bond->params.keep_all); > +} > + > +static ssize_t bonding_store_keep(struct device *d, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int i, new_value, ret = count; > + struct bonding *bond = to_bond(d); > + struct slave *slave; > + > + if (sscanf(buf, "%d", &new_value) != 1) { > + pr_err("%s: no keep_all value specified.\n", > + bond->dev->name); > + ret = -EINVAL; > + goto out; > + } > + > + if (new_value == bond->params.keep_all) > + goto out; > + > + if ((new_value == 0) || (new_value == 1)) { > + bond->params.keep_all = new_value; > + } else { > + pr_info("%s: Ignoring invalid keep_all value %d.\n", > + bond->dev->name, new_value); > + ret = -EINVAL; > + goto out; > + } > + > + bond_for_each_slave(bond, slave, i) { > + if (slave->state == BOND_STATE_BACKUP) { > + if (new_value) > + slave->dev->priv_flags &= ~IFF_SLAVE_INACTIVE; > + else > + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > + } > + } > +out: > + return count; > +} > +static DEVICE_ATTR(keep_all, S_IRUGO | S_IWUSR, > + bonding_show_keep, bonding_store_keep); > + > > > static struct attribute *per_bond_attrs[] = { > @@ -1499,6 +1551,7 @@ static struct attribute *per_bond_attrs[] = { > &dev_attr_ad_actor_key.attr, > &dev_attr_ad_partner_key.attr, > &dev_attr_ad_partner_mac.attr, > + &dev_attr_keep_all.attr, > NULL, > }; > > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h > index 2aa3367..ef7969b 100644 > --- a/drivers/net/bonding/bonding.h > +++ b/drivers/net/bonding/bonding.h > @@ -131,6 +131,7 @@ struct bond_params { > char primary[IFNAMSIZ]; > int primary_reselect; > __be32 arp_targets[BOND_MAX_ARP_TARGETS]; > + int keep_all; > }; > > struct bond_parm_tbl { > @@ -291,7 +292,8 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) > struct bonding *bond = netdev_priv(slave->dev->master); > if (!bond_is_lb(bond)) > slave->state = BOND_STATE_BACKUP; > - slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > + if (!bond->params.keep_all) > + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > if (slave_do_arp_validate(bond, slave)) > slave->dev->priv_flags |= IFF_SLAVE_NEEDARP; > } > > > --- > -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 18, 2010 at 08:57:05AM -0400, Andy Gospodarek wrote: > On Mon, May 17, 2010 at 9:21 PM, Jay Vosburgh <fubar@us.ibm.com> wrote: > > Jay Vosburgh <fubar@us.ibm.com> wrote: > > [...] > >> For your patch, I'm exploring the idea of not setting > >>IFF_SLAVE_INACTIVE on "inactive" slaves for an "all_slaves_active" > >>option (I think that's a more descriptive name than "keep_all") instead > >>of adding a new KEEP_ALL flag bit to priv_flags. Did you consider this > >>methodology and exclude it for some reason? > > > > Following up to myself, I coded up approximately what I was > > talking about. This doesn't require the extra priv_flag, and the sysfs > > _store is a little more complicated, but this appears to work (testing > > with ping -f after clearing the switch's MAC table to induce traffic > > flooding). I didn't change the option name from "keep_all" here, but as > > far as the functionality goes, this seems to do what I think you want it > > to. > > > > I can test it here to be sure, but overall this seems like a fine > approach. The IFF_SLAVE_INACTIVE doesn't seem to be used for much > other than duplicate suppression at this point, so it seems like a > logical choice. > > As for the original name 'keep_all,' I tried to use something that > user would find easy to understand. Obviously not all users think > alike. :-) > > Sorry, Just trying to keep this from falling off my radar. Andy, any test results on this? Thanks Neil > > -J > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 5e12462..c80d2ff 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -106,6 +106,7 @@ static int arp_interval = BOND_LINK_ARP_INTERV; > > static char *arp_ip_target[BOND_MAX_ARP_TARGETS]; > > static char *arp_validate; > > static char *fail_over_mac; > > +static int keep_all = 0; > > static struct bond_params bonding_defaults; > > > > module_param(max_bonds, int, 0); > > @@ -155,6 +156,9 @@ module_param(arp_validate, charp, 0); > > MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes: none (default), active, backup or all"); > > module_param(fail_over_mac, charp, 0); > > MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to the same MAC. none (default), active or follow"); > > +module_param(keep_all, int, 0); > > +MODULE_PARM_DESC(keep_all, "Keep all frames received on an interface" > > + "0 for never (default), 1 for always."); > > > > /*----------------------------- Global variables ----------------------------*/ > > > > @@ -4756,6 +4760,13 @@ static int bond_check_params(struct bond_params *params) > > } > > } > > > > + if ((keep_all != 0) && (keep_all != 1)) { > > + pr_warning("Warning: keep_all module parameter (%d), " > > + "not of valid value (0/1), so it was set to " > > + "0\n", keep_all); > > + keep_all = 0; > > + } > > + > > /* reset values for TLB/ALB */ > > if ((bond_mode == BOND_MODE_TLB) || > > (bond_mode == BOND_MODE_ALB)) { > > @@ -4926,6 +4937,7 @@ static int bond_check_params(struct bond_params *params) > > params->primary[0] = 0; > > params->primary_reselect = primary_reselect_value; > > params->fail_over_mac = fail_over_mac_value; > > + params->keep_all = keep_all; > > > > if (primary) { > > strncpy(params->primary, primary, IFNAMSIZ); > > diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c > > index b8bec08..73b3c03 100644 > > --- a/drivers/net/bonding/bond_sysfs.c > > +++ b/drivers/net/bonding/bond_sysfs.c > > @@ -339,7 +339,6 @@ out: > > > > static DEVICE_ATTR(slaves, S_IRUGO | S_IWUSR, bonding_show_slaves, > > bonding_store_slaves); > > - > > /* > > * Show and set the bonding mode. The bond interface must be down to > > * change the mode. > > @@ -1472,6 +1471,59 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d, > > } > > static DEVICE_ATTR(ad_partner_mac, S_IRUGO, bonding_show_ad_partner_mac, NULL); > > > > +/* > > + * Show and set the keep_all flag. > > + */ > > +static ssize_t bonding_show_keep(struct device *d, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct bonding *bond = to_bond(d); > > + > > + return sprintf(buf, "%d\n", bond->params.keep_all); > > +} > > + > > +static ssize_t bonding_store_keep(struct device *d, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int i, new_value, ret = count; > > + struct bonding *bond = to_bond(d); > > + struct slave *slave; > > + > > + if (sscanf(buf, "%d", &new_value) != 1) { > > + pr_err("%s: no keep_all value specified.\n", > > + bond->dev->name); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + if (new_value == bond->params.keep_all) > > + goto out; > > + > > + if ((new_value == 0) || (new_value == 1)) { > > + bond->params.keep_all = new_value; > > + } else { > > + pr_info("%s: Ignoring invalid keep_all value %d.\n", > > + bond->dev->name, new_value); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + bond_for_each_slave(bond, slave, i) { > > + if (slave->state == BOND_STATE_BACKUP) { > > + if (new_value) > > + slave->dev->priv_flags &= ~IFF_SLAVE_INACTIVE; > > + else > > + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > > + } > > + } > > +out: > > + return count; > > +} > > +static DEVICE_ATTR(keep_all, S_IRUGO | S_IWUSR, > > + bonding_show_keep, bonding_store_keep); > > + > > > > > > static struct attribute *per_bond_attrs[] = { > > @@ -1499,6 +1551,7 @@ static struct attribute *per_bond_attrs[] = { > > &dev_attr_ad_actor_key.attr, > > &dev_attr_ad_partner_key.attr, > > &dev_attr_ad_partner_mac.attr, > > + &dev_attr_keep_all.attr, > > NULL, > > }; > > > > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h > > index 2aa3367..ef7969b 100644 > > --- a/drivers/net/bonding/bonding.h > > +++ b/drivers/net/bonding/bonding.h > > @@ -131,6 +131,7 @@ struct bond_params { > > char primary[IFNAMSIZ]; > > int primary_reselect; > > __be32 arp_targets[BOND_MAX_ARP_TARGETS]; > > + int keep_all; > > }; > > > > struct bond_parm_tbl { > > @@ -291,7 +292,8 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) > > struct bonding *bond = netdev_priv(slave->dev->master); > > if (!bond_is_lb(bond)) > > slave->state = BOND_STATE_BACKUP; > > - slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > > + if (!bond->params.keep_all) > > + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; > > if (slave_do_arp_validate(bond, slave)) > > slave->dev->priv_flags |= IFF_SLAVE_NEEDARP; > > } > > > > > > --- > > -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 17, 2010 at 06:21:54PM -0700, Jay Vosburgh wrote: > Jay Vosburgh <fubar@us.ibm.com> wrote: > [...] > > For your patch, I'm exploring the idea of not setting > >IFF_SLAVE_INACTIVE on "inactive" slaves for an "all_slaves_active" > >option (I think that's a more descriptive name than "keep_all") instead > >of adding a new KEEP_ALL flag bit to priv_flags. Did you consider this > >methodology and exclude it for some reason? > > Following up to myself, I coded up approximately what I was > talking about. This doesn't require the extra priv_flag, and the sysfs > _store is a little more complicated, but this appears to work (testing > with ping -f after clearing the switch's MAC table to induce traffic > flooding). I didn't change the option name from "keep_all" here, but as > far as the functionality goes, this seems to do what I think you want it > to. > > -J This looks good to me, Jay. I tested the patch here along with the patch that started this thread and it works as expected. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> It seems like you were willing to take the patch that started this thread rather than a change that adds a new mode. If we agree that this is the correct direction, can you take a look at the patch and decide if you are willing to ACK it as-is or if more changes are needed? Thanks, -andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 24, 2010 at 11:31:08AM -0400, Andy Gospodarek wrote: > On Mon, May 17, 2010 at 06:21:54PM -0700, Jay Vosburgh wrote: > > Jay Vosburgh <fubar@us.ibm.com> wrote: > > [...] > > > For your patch, I'm exploring the idea of not setting > > >IFF_SLAVE_INACTIVE on "inactive" slaves for an "all_slaves_active" > > >option (I think that's a more descriptive name than "keep_all") instead > > >of adding a new KEEP_ALL flag bit to priv_flags. Did you consider this > > >methodology and exclude it for some reason? > > > > Following up to myself, I coded up approximately what I was > > talking about. This doesn't require the extra priv_flag, and the sysfs > > _store is a little more complicated, but this appears to work (testing > > with ping -f after clearing the switch's MAC table to induce traffic > > flooding). I didn't change the option name from "keep_all" here, but as > > far as the functionality goes, this seems to do what I think you want it > > to. > > > > -J > > This looks good to me, Jay. I tested the patch here along with the > patch that started this thread and it works as expected. > > Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > > It seems like you were willing to take the patch that started this > thread rather than a change that adds a new mode. If we agree that this > is the correct direction, can you take a look at the patch and decide if > you are willing to ACK it as-is or if more changes are needed? > > Thanks, > > -andy > Awesome, thanks Andy! Neil > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 5e12462..c80d2ff 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -106,6 +106,7 @@ static int arp_interval = BOND_LINK_ARP_INTERV; static char *arp_ip_target[BOND_MAX_ARP_TARGETS]; static char *arp_validate; static char *fail_over_mac; +static int keep_all = 0; static struct bond_params bonding_defaults; module_param(max_bonds, int, 0); @@ -155,6 +156,9 @@ module_param(arp_validate, charp, 0); MODULE_PARM_DESC(arp_validate, "validate src/dst of ARP probes: none (default), active, backup or all"); module_param(fail_over_mac, charp, 0); MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to the same MAC. none (default), active or follow"); +module_param(keep_all, int, 0); +MODULE_PARM_DESC(keep_all, "Keep all frames received on an interface" + "0 for never (default), 1 for always."); /*----------------------------- Global variables ----------------------------*/ @@ -4756,6 +4760,13 @@ static int bond_check_params(struct bond_params *params) } } + if ((keep_all != 0) && (keep_all != 1)) { + pr_warning("Warning: keep_all module parameter (%d), " + "not of valid value (0/1), so it was set to " + "0\n", keep_all); + keep_all = 0; + } + /* reset values for TLB/ALB */ if ((bond_mode == BOND_MODE_TLB) || (bond_mode == BOND_MODE_ALB)) { @@ -4926,6 +4937,7 @@ static int bond_check_params(struct bond_params *params) params->primary[0] = 0; params->primary_reselect = primary_reselect_value; params->fail_over_mac = fail_over_mac_value; + params->keep_all = keep_all; if (primary) { strncpy(params->primary, primary, IFNAMSIZ); diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index b8bec08..73b3c03 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -339,7 +339,6 @@ out: static DEVICE_ATTR(slaves, S_IRUGO | S_IWUSR, bonding_show_slaves, bonding_store_slaves); - /* * Show and set the bonding mode. The bond interface must be down to * change the mode. @@ -1472,6 +1471,59 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d, } static DEVICE_ATTR(ad_partner_mac, S_IRUGO, bonding_show_ad_partner_mac, NULL); +/* + * Show and set the keep_all flag. + */ +static ssize_t bonding_show_keep(struct device *d, + struct device_attribute *attr, + char *buf) +{ + struct bonding *bond = to_bond(d); + + return sprintf(buf, "%d\n", bond->params.keep_all); +} + +static ssize_t bonding_store_keep(struct device *d, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int i, new_value, ret = count; + struct bonding *bond = to_bond(d); + struct slave *slave; + + if (sscanf(buf, "%d", &new_value) != 1) { + pr_err("%s: no keep_all value specified.\n", + bond->dev->name); + ret = -EINVAL; + goto out; + } + + if (new_value == bond->params.keep_all) + goto out; + + if ((new_value == 0) || (new_value == 1)) { + bond->params.keep_all = new_value; + } else { + pr_info("%s: Ignoring invalid keep_all value %d.\n", + bond->dev->name, new_value); + ret = -EINVAL; + goto out; + } + + bond_for_each_slave(bond, slave, i) { + if (slave->state == BOND_STATE_BACKUP) { + if (new_value) + slave->dev->priv_flags &= ~IFF_SLAVE_INACTIVE; + else + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; + } + } +out: + return count; +} +static DEVICE_ATTR(keep_all, S_IRUGO | S_IWUSR, + bonding_show_keep, bonding_store_keep); + static struct attribute *per_bond_attrs[] = { @@ -1499,6 +1551,7 @@ static struct attribute *per_bond_attrs[] = { &dev_attr_ad_actor_key.attr, &dev_attr_ad_partner_key.attr, &dev_attr_ad_partner_mac.attr, + &dev_attr_keep_all.attr, NULL, }; diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 2aa3367..ef7969b 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -131,6 +131,7 @@ struct bond_params { char primary[IFNAMSIZ]; int primary_reselect; __be32 arp_targets[BOND_MAX_ARP_TARGETS]; + int keep_all; }; struct bond_parm_tbl { @@ -291,7 +292,8 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) struct bonding *bond = netdev_priv(slave->dev->master); if (!bond_is_lb(bond)) slave->state = BOND_STATE_BACKUP; - slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; + if (!bond->params.keep_all) + slave->dev->priv_flags |= IFF_SLAVE_INACTIVE; if (slave_do_arp_validate(bond, slave)) slave->dev->priv_flags |= IFF_SLAVE_NEEDARP; }