| Message ID | 1421423848-414-2-git-send-email-jtoppins@cumulusnetworks.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 01/16/2015 04:57 PM, Jonathan Toppins wrote: > From: Scott Feldman <sfeldma@cumulusnetworks.com> > > Bonding driver parameter min_links is now used to signal upper-level > protocols of bond status. The way it works is if the total number of > active members in slaves drops below min_links, the bond link carrier > will go down, signaling upper levels that bond is inactive. When active > members returns to >= min_links, bond link carrier will go up (RUNNING), > and protocols can resume. When bond is carrier down, member ports are > in stp fwd state blocked (rather than normal disabled state), so > low-level ctrl protocols (LACP) can still get in and be processed by > bonding driver. > > LACP will still do it's job while bond is carrier off, and if bond members > become active, bond carrier will be turned back on, signaling higher-level > protocols that bond is viable. > > Suggested setting of min_links is 1, rather than the default of zero. > Using min_links=1 says that at least 1 slave must be active within bond > for bond to be carrier on. > > Finally, when min_links bonding option is changed update carrier status. > > Cc: Scott Feldman <sfeldma@gmail.com> > Cc: Andy Gospodarek <gospo@cumulusnetworks.com> > Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com> > --- > drivers/net/bonding/bond_3ad.c | 18 ++++++++++++++---- > drivers/net/bonding/bond_main.c | 2 +- > drivers/net/bonding/bond_options.c | 1 + > include/net/bonding.h | 1 + > 4 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c > index 8baa87d..e9b706f 100644 > --- a/drivers/net/bonding/bond_3ad.c > +++ b/drivers/net/bonding/bond_3ad.c > @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg) > static inline void __disable_port(struct port *port) > { > bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER); > + bond_3ad_set_carrier(port->slave->bond); > } > > /** > @@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port) > { > struct slave *slave = port->slave; > > - if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) > + if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) { While at it please remove the extra ( ) around slave->link == BOND_LINK_UP. > bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER); > + bond_3ad_set_carrier(slave->bond); > + } > } > > /** > @@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link) > int bond_3ad_set_carrier(struct bonding *bond) > { > struct aggregator *active; > - struct slave *first_slave; > + struct slave *first_slave, *slave; > + struct list_head *iter; > int ret = 1; > + int active_slaves = 0; > > rcu_read_lock(); > first_slave = bond_first_slave_rcu(bond); > @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond) > ret = 0; > goto out; > } > + > + bond_for_each_slave_rcu(bond, slave, iter) > + if (SLAVE_AD_INFO(slave)->aggregator.is_active) > + active_slaves++; > + > active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator)); > - if (active) { > + if (active && __agg_has_partner(active)) { > /* are enough slaves available to consider link up? */ > - if (active->num_of_ports < bond->params.min_links) { > + if (active_slaves < bond->params.min_links) { > if (netif_carrier_ok(bond->dev)) { > netif_carrier_off(bond->dev); > goto out; > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 0dceba1..02ffedb 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev, > * > * Returns zero if carrier state does not change, nonzero if it does. > */ > -static int bond_set_carrier(struct bonding *bond) > +int bond_set_carrier(struct bonding *bond) > { > struct list_head *iter; > struct slave *slave; > diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c > index 9bd538d4..4df2894 100644 > --- a/drivers/net/bonding/bond_options.c > +++ b/drivers/net/bonding/bond_options.c > @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond, > netdev_info(bond->dev, "Setting min links value to %llu\n", > newval->value); > bond->params.min_links = newval->value; > + bond_set_carrier(bond); > > return 0; > } > diff --git a/include/net/bonding.h b/include/net/bonding.h > index 983a94b..29f53ea 100644 > --- a/include/net/bonding.h > +++ b/include/net/bonding.h > @@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave); > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev); > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); > u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb); > +int bond_set_carrier(struct bonding *bond); > void bond_select_active_slave(struct bonding *bond); > void bond_change_active_slave(struct bonding *bond, struct slave *new_active); > void bond_create_debugfs(void); > -- 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 1/19/15 2:27 PM, Nikolay Aleksandrov wrote: > On 01/16/2015 04:57 PM, Jonathan Toppins wrote: >> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >> index 8baa87d..e9b706f 100644 >> --- a/drivers/net/bonding/bond_3ad.c >> +++ b/drivers/net/bonding/bond_3ad.c >> @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg) >> static inline void __disable_port(struct port *port) >> { >> bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER); >> + bond_3ad_set_carrier(port->slave->bond); >> } >> >> /** >> @@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port) >> { >> struct slave *slave = port->slave; >> >> - if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) >> + if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) { > While at it please remove the extra ( ) around slave->link == BOND_LINK_UP. > Ack. -- 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
Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote: >From: Scott Feldman <sfeldma@cumulusnetworks.com> > >Bonding driver parameter min_links is now used to signal upper-level >protocols of bond status. The way it works is if the total number of >active members in slaves drops below min_links, the bond link carrier >will go down, signaling upper levels that bond is inactive. When active >members returns to >= min_links, bond link carrier will go up (RUNNING), >and protocols can resume. When bond is carrier down, member ports are >in stp fwd state blocked (rather than normal disabled state), so >low-level ctrl protocols (LACP) can still get in and be processed by >bonding driver. Presuming that "stp" is Spanning Tree, is the last sentence above actually describing the behavior of a bridge port when a bond is the member of the bridge? I'm not sure I understand what "member ports" refers to (bridge ports or bonding slaves). One code comment, below... >LACP will still do it's job while bond is carrier off, and if bond members >become active, bond carrier will be turned back on, signaling higher-level >protocols that bond is viable. > >Suggested setting of min_links is 1, rather than the default of zero. >Using min_links=1 says that at least 1 slave must be active within bond >for bond to be carrier on. > >Finally, when min_links bonding option is changed update carrier status. > >Cc: Scott Feldman <sfeldma@gmail.com> >Cc: Andy Gospodarek <gospo@cumulusnetworks.com> >Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com> >--- > drivers/net/bonding/bond_3ad.c | 18 ++++++++++++++---- > drivers/net/bonding/bond_main.c | 2 +- > drivers/net/bonding/bond_options.c | 1 + > include/net/bonding.h | 1 + > 4 files changed, 17 insertions(+), 5 deletions(-) > >diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c >index 8baa87d..e9b706f 100644 >--- a/drivers/net/bonding/bond_3ad.c >+++ b/drivers/net/bonding/bond_3ad.c >@@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg) > static inline void __disable_port(struct port *port) > { > bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER); >+ bond_3ad_set_carrier(port->slave->bond); > } > > /** >@@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port) > { > struct slave *slave = port->slave; > >- if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) >+ if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) { > bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER); >+ bond_3ad_set_carrier(slave->bond); >+ } > } > > /** >@@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link) > int bond_3ad_set_carrier(struct bonding *bond) > { > struct aggregator *active; >- struct slave *first_slave; >+ struct slave *first_slave, *slave; >+ struct list_head *iter; > int ret = 1; >+ int active_slaves = 0; > > rcu_read_lock(); > first_slave = bond_first_slave_rcu(bond); >@@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond) > ret = 0; > goto out; > } >+ >+ bond_for_each_slave_rcu(bond, slave, iter) >+ if (SLAVE_AD_INFO(slave)->aggregator.is_active) >+ active_slaves++; >+ > active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator)); >- if (active) { >+ if (active && __agg_has_partner(active)) { Why "__agg_has_partner"? Since the "else" of this clause is: } else if (netif_carrier_ok(bond->dev)) { netif_carrier_off(bond->dev); } I'm wondering if this will do the right thing for the case that there are no LACP partners at all (e.g., the switch ports do not have LACP enabled), in which case the active aggregator should be a single "individual" port as a fallback, but will not have a partner. -J > /* are enough slaves available to consider link up? */ >- if (active->num_of_ports < bond->params.min_links) { >+ if (active_slaves < bond->params.min_links) { > if (netif_carrier_ok(bond->dev)) { > netif_carrier_off(bond->dev); > goto out; >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 0dceba1..02ffedb 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev, > * > * Returns zero if carrier state does not change, nonzero if it does. > */ >-static int bond_set_carrier(struct bonding *bond) >+int bond_set_carrier(struct bonding *bond) > { > struct list_head *iter; > struct slave *slave; >diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c >index 9bd538d4..4df2894 100644 >--- a/drivers/net/bonding/bond_options.c >+++ b/drivers/net/bonding/bond_options.c >@@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond, > netdev_info(bond->dev, "Setting min links value to %llu\n", > newval->value); > bond->params.min_links = newval->value; >+ bond_set_carrier(bond); > > return 0; > } >diff --git a/include/net/bonding.h b/include/net/bonding.h >index 983a94b..29f53ea 100644 >--- a/include/net/bonding.h >+++ b/include/net/bonding.h >@@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave); > int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev); > int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); > u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb); >+int bond_set_carrier(struct bonding *bond); > void bond_select_active_slave(struct bonding *bond); > void bond_change_active_slave(struct bonding *bond, struct slave *new_active); > void bond_create_debugfs(void); >-- >1.7.10.4 --- -Jay Vosburgh, jay.vosburgh@canonical.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 1/19/15 4:16 PM, Jay Vosburgh wrote: > Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote: > >> From: Scott Feldman <sfeldma@cumulusnetworks.com> >> >> Bonding driver parameter min_links is now used to signal upper-level >> protocols of bond status. The way it works is if the total number of >> active members in slaves drops below min_links, the bond link carrier >> will go down, signaling upper levels that bond is inactive. When active >> members returns to >= min_links, bond link carrier will go up (RUNNING), >> and protocols can resume. When bond is carrier down, member ports are >> in stp fwd state blocked (rather than normal disabled state), so >> low-level ctrl protocols (LACP) can still get in and be processed by >> bonding driver. > > Presuming that "stp" is Spanning Tree, is the last sentence > above actually describing the behavior of a bridge port when a bond is > the member of the bridge? I'm not sure I understand what "member ports" > refers to (bridge ports or bonding slaves). Ack, maybe replacing the last sentence with something like: When bond is carrier down, the slave ports are only forwarding low-level control protocols (e.g. LACP PDU) and discarding all other packets. >> @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond) >> ret = 0; >> goto out; >> } >> + >> + bond_for_each_slave_rcu(bond, slave, iter) >> + if (SLAVE_AD_INFO(slave)->aggregator.is_active) >> + active_slaves++; >> + >> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator)); >> - if (active) { >> + if (active && __agg_has_partner(active)) { > > Why "__agg_has_partner"? Since the "else" of this clause is: > > } else if (netif_carrier_ok(bond->dev)) { > netif_carrier_off(bond->dev); > } > > I'm wondering if this will do the right thing for the case that > there are no LACP partners at all (e.g., the switch ports do not have > LACP enabled), in which case the active aggregator should be a single > "individual" port as a fallback, but will not have a partner. > > -J > I see your point. The initial thinking was the logical bond carrier should not be brought up until the bond has a partner and is ready to pass traffic, otherwise we start blackholing frames. Looking over the code it seems the aggregator.is_individual flag is only set to true when a slave is in half-duplex, this seems odd? My initial thinking to alleviate the concern is something like the following: if (active && !SLAVE_AD_INFO(slave)->aggregator.is_individual && __agg_has_partner(active)) { /* set carrier based on min_links */ } else if (active && SLAVE_AD_INFO(slave)->aggregator.is_individual) { /* set bond carrier state according to carrier state of slave */ } else if (netif_carrier_ok(bond->dev)) { netif_carrier_off(bond->dev); } Maybe I am missing something and there is a simpler option. Thinking about how to validate this, it seems having a bond with two slaves and both slaves in half-duplex will force an aggregator that is individual to be selected. Thoughts? -Jon -- 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 1/19/15 5:15 PM, Scott Feldman wrote: > > On Jan 16, 2015 7:57 AM, "Jonathan Toppins" > <jtoppins@cumulusnetworks.com <mailto:jtoppins@cumulusnetworks.com>> wrote: > > > > From: Scott Feldman <sfeldma@cumulusnetworks.com > <mailto:sfeldma@cumulusnetworks.com>> > > Hmmmm, this feels a little creepy, sending a patch from my past. I do > appreciate the attribution, but I can't ACK or NACK or even comment as > I've lost the context around this patch. And I'm afraid I'll break out > in hives if I look at the bonding code again, so if you don't mind, > please remove me from this patch. Thank you. > Have removed you for v2 of series, should we consider this a blanket statement for any other patches we might be releasing that you had a hand in? There were a few ;) -- 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_3ad.c b/drivers/net/bonding/bond_3ad.c index 8baa87d..e9b706f 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -189,6 +189,7 @@ static inline int __agg_has_partner(struct aggregator *agg) static inline void __disable_port(struct port *port) { bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER); + bond_3ad_set_carrier(port->slave->bond); } /** @@ -199,8 +200,10 @@ static inline void __enable_port(struct port *port) { struct slave *slave = port->slave; - if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) + if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave)) { bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER); + bond_3ad_set_carrier(slave->bond); + } } /** @@ -2372,8 +2375,10 @@ void bond_3ad_handle_link_change(struct slave *slave, char link) int bond_3ad_set_carrier(struct bonding *bond) { struct aggregator *active; - struct slave *first_slave; + struct slave *first_slave, *slave; + struct list_head *iter; int ret = 1; + int active_slaves = 0; rcu_read_lock(); first_slave = bond_first_slave_rcu(bond); @@ -2381,10 +2386,15 @@ int bond_3ad_set_carrier(struct bonding *bond) ret = 0; goto out; } + + bond_for_each_slave_rcu(bond, slave, iter) + if (SLAVE_AD_INFO(slave)->aggregator.is_active) + active_slaves++; + active = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator)); - if (active) { + if (active && __agg_has_partner(active)) { /* are enough slaves available to consider link up? */ - if (active->num_of_ports < bond->params.min_links) { + if (active_slaves < bond->params.min_links) { if (netif_carrier_ok(bond->dev)) { netif_carrier_off(bond->dev); goto out; diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 0dceba1..02ffedb 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -334,7 +334,7 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev, * * Returns zero if carrier state does not change, nonzero if it does. */ -static int bond_set_carrier(struct bonding *bond) +int bond_set_carrier(struct bonding *bond) { struct list_head *iter; struct slave *slave; diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 9bd538d4..4df2894 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -1181,6 +1181,7 @@ static int bond_option_min_links_set(struct bonding *bond, netdev_info(bond->dev, "Setting min links value to %llu\n", newval->value); bond->params.min_links = newval->value; + bond_set_carrier(bond); return 0; } diff --git a/include/net/bonding.h b/include/net/bonding.h index 983a94b..29f53ea 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -525,6 +525,7 @@ void bond_sysfs_slave_del(struct slave *slave); int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev); int bond_release(struct net_device *bond_dev, struct net_device *slave_dev); u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb); +int bond_set_carrier(struct bonding *bond); void bond_select_active_slave(struct bonding *bond); void bond_change_active_slave(struct bonding *bond, struct slave *new_active); void bond_create_debugfs(void);