| Message ID | 1392648088-21336-3-git-send-email-vfalico@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 02/17/2014 03:41 PM, Veaceslav Falico wrote: > Currently it's disabled because it's sometimes hard, in typical configs, to > make it work - because of the nature how the loadbalance modes work - as > it's hard to deliver valid arp replies to correct slaves by the switch. > > However we still can use arp_validation in loadbalance with several other > configs, per example with arp_validate == 2 for backup with one broadcast > domain, without the switch(es) doing any balancing - this way we'd be (a > bit more) sure that the slave is up. > > So, enable it to let users decide which one works/suits them best. Also > remove the mode limitation from BOND_OPT_ARP_VALIDATE. > > CC: Jay Vosburgh <fubar@us.ibm.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > Documentation/networking/bonding.txt | 6 +++--- > drivers/net/bonding/bond_main.c | 4 ---- > drivers/net/bonding/bond_options.c | 1 - > 3 files changed, 3 insertions(+), 8 deletions(-) > > diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt > index 5cdb229..96b4ad8 100644 > --- a/Documentation/networking/bonding.txt > +++ b/Documentation/networking/bonding.txt > @@ -270,9 +270,9 @@ arp_ip_target > arp_validate > > Specifies whether or not ARP probes and replies should be > - validated in the active-backup mode. This causes the ARP > - monitor to examine the incoming ARP requests and replies, and > - only consider a slave to be up if it is receiving the > + validated in any mode that supports arp monitoring. This causes > + the ARP monitor to examine the incoming ARP requests and replies, > + and only consider a slave to be up if it is receiving the > appropriate ARP traffic. > > Possible values are: > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 3c50bec..91c0248 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -4183,10 +4183,6 @@ static int bond_check_params(struct bond_params *params) > } > > if (arp_validate) { > - if (bond_mode != BOND_MODE_ACTIVEBACKUP) { > - pr_err("arp_validate only supported in active-backup mode\n"); > - return -EINVAL; > - } > if (!arp_interval) { > pr_err("arp_validate requires arp_interval\n"); > return -EINVAL; > diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > index f3eb44d..d566cab 100644 > --- a/drivers/net/bonding/bond_options.c > +++ b/drivers/net/bonding/bond_options.c > @@ -151,7 +151,6 @@ static struct bond_option bond_opts[] = { > .id = BOND_OPT_ARP_VALIDATE, > .name = "arp_validate", > .desc = "validate src/dst of ARP probes", > - .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ACTIVEBACKUP)), > .values = bond_arp_validate_tbl, > .set = bond_option_arp_validate_set > }, > Sorry for not noticing before, but I just remembered the reason why all the checks are there when changing the recv_probe, especially from the options. The problem was that lacp mode also uses the recv_probe, so after this patch we can overwrite the recv_probe while it's running in lacp mode, because bond_option_arp_validate_set changes the recv_probe. We can actually only set it to NULL because arp_interval can't be set in those modes, but still we can break the mode and there'll be need for at least down/up cycle to fix it. I think rlb also uses the recv_probe so this applies to it as well. Nik -- 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, Feb 17, 2014 at 04:22:21PM +0100, Nikolay Aleksandrov wrote: >On 02/17/2014 03:41 PM, Veaceslav Falico wrote: >> Currently it's disabled because it's sometimes hard, in typical configs, to >> make it work - because of the nature how the loadbalance modes work - as >> it's hard to deliver valid arp replies to correct slaves by the switch. >> >> However we still can use arp_validation in loadbalance with several other >> configs, per example with arp_validate == 2 for backup with one broadcast >> domain, without the switch(es) doing any balancing - this way we'd be (a >> bit more) sure that the slave is up. >> >> So, enable it to let users decide which one works/suits them best. Also >> remove the mode limitation from BOND_OPT_ARP_VALIDATE. >> >> CC: Jay Vosburgh <fubar@us.ibm.com> >> CC: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >> --- >> Documentation/networking/bonding.txt | 6 +++--- >> drivers/net/bonding/bond_main.c | 4 ---- >> drivers/net/bonding/bond_options.c | 1 - >> 3 files changed, 3 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt >> index 5cdb229..96b4ad8 100644 >> --- a/Documentation/networking/bonding.txt >> +++ b/Documentation/networking/bonding.txt >> @@ -270,9 +270,9 @@ arp_ip_target >> arp_validate >> >> Specifies whether or not ARP probes and replies should be >> - validated in the active-backup mode. This causes the ARP >> - monitor to examine the incoming ARP requests and replies, and >> - only consider a slave to be up if it is receiving the >> + validated in any mode that supports arp monitoring. This causes >> + the ARP monitor to examine the incoming ARP requests and replies, >> + and only consider a slave to be up if it is receiving the >> appropriate ARP traffic. >> >> Possible values are: >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 3c50bec..91c0248 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -4183,10 +4183,6 @@ static int bond_check_params(struct bond_params *params) >> } >> >> if (arp_validate) { >> - if (bond_mode != BOND_MODE_ACTIVEBACKUP) { >> - pr_err("arp_validate only supported in active-backup mode\n"); >> - return -EINVAL; >> - } >> if (!arp_interval) { >> pr_err("arp_validate requires arp_interval\n"); >> return -EINVAL; >> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >> index f3eb44d..d566cab 100644 >> --- a/drivers/net/bonding/bond_options.c >> +++ b/drivers/net/bonding/bond_options.c >> @@ -151,7 +151,6 @@ static struct bond_option bond_opts[] = { >> .id = BOND_OPT_ARP_VALIDATE, >> .name = "arp_validate", >> .desc = "validate src/dst of ARP probes", >> - .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ACTIVEBACKUP)), >> .values = bond_arp_validate_tbl, >> .set = bond_option_arp_validate_set >> }, >> >Sorry for not noticing before, but I just remembered the reason why all the >checks are there when changing the recv_probe, especially from the options. >The problem was that lacp mode also uses the recv_probe, so after this patch we >can overwrite the recv_probe while it's running in lacp mode, because >bond_option_arp_validate_set changes the recv_probe. We can actually only set it >to NULL because arp_interval can't be set in those modes, but still we can break >the mode and there'll be need for at least down/up cycle to fix it. >I think rlb also uses the recv_probe so this applies to it as well. Yep, missed that too, thanks a lot Nik. It should just be the same as for BOND_OPT_ARP_INTERVAL: 177 .unsuppmodes = BIT(BOND_MODE_8023AD) | BIT(BOND_MODE_TLB) | 178 BIT(BOND_MODE_ALB), which makes sense - as in those modes it's indeed more than useless. I'll change it and send v5. > >Nik > -- 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/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt index 5cdb229..96b4ad8 100644 --- a/Documentation/networking/bonding.txt +++ b/Documentation/networking/bonding.txt @@ -270,9 +270,9 @@ arp_ip_target arp_validate Specifies whether or not ARP probes and replies should be - validated in the active-backup mode. This causes the ARP - monitor to examine the incoming ARP requests and replies, and - only consider a slave to be up if it is receiving the + validated in any mode that supports arp monitoring. This causes + the ARP monitor to examine the incoming ARP requests and replies, + and only consider a slave to be up if it is receiving the appropriate ARP traffic. Possible values are: diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 3c50bec..91c0248 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4183,10 +4183,6 @@ static int bond_check_params(struct bond_params *params) } if (arp_validate) { - if (bond_mode != BOND_MODE_ACTIVEBACKUP) { - pr_err("arp_validate only supported in active-backup mode\n"); - return -EINVAL; - } if (!arp_interval) { pr_err("arp_validate requires arp_interval\n"); return -EINVAL; diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index f3eb44d..d566cab 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -151,7 +151,6 @@ static struct bond_option bond_opts[] = { .id = BOND_OPT_ARP_VALIDATE, .name = "arp_validate", .desc = "validate src/dst of ARP probes", - .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_ACTIVEBACKUP)), .values = bond_arp_validate_tbl, .set = bond_option_arp_validate_set },
Currently it's disabled because it's sometimes hard, in typical configs, to make it work - because of the nature how the loadbalance modes work - as it's hard to deliver valid arp replies to correct slaves by the switch. However we still can use arp_validation in loadbalance with several other configs, per example with arp_validate == 2 for backup with one broadcast domain, without the switch(es) doing any balancing - this way we'd be (a bit more) sure that the slave is up. So, enable it to let users decide which one works/suits them best. Also remove the mode limitation from BOND_OPT_ARP_VALIDATE. CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- Documentation/networking/bonding.txt | 6 +++--- drivers/net/bonding/bond_main.c | 4 ---- drivers/net/bonding/bond_options.c | 1 - 3 files changed, 3 insertions(+), 8 deletions(-)