I agree that this is much easier to understand. It looks good to me.
Thank you.
On Fri, Jan 13, 2012 at 11:56:49AM -0800, Ethan Jackson wrote:
> Here's the original version of the patch that I'd written. I had remvoed the
> LACP_CONFIGURED enum because I don't think it's strictly necessary. The same
> behavior can be achieved by dropping all traffic on non-enabled slaves in the
> admissibility function. However, I think this patch is *much* easier to
> understand so I'm proposing it now.
>
> Ethan
>
> ---
> In the original Open vSwitch LACP implementation, when no slaves
> found a LACP partner, the LACP module would attach all of them.
> This allowed the LACP bond to fall back to a standard bond when
> partnered with a non-LACP switch. In practice, this has caused
> confusion with marginal benefit, so this feature is removed with
> this patch.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
> ---
> NEWS | 3 ++
> lib/bond.c | 91
> ++++++++++++++++++++++++++++--------------------
> lib/bond.h | 3 +-
> lib/lacp.c | 30 ++++++++--------
> lib/lacp.h | 8 ++++-
> ofproto/ofproto-dpif.c | 2 +-
> tests/lacp.at | 2 +-
> vswitchd/vswitch.xml | 8 ++--
> 8 files changed, 86 insertions(+), 61 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index b628e29..8ff87c4 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,8 @@
> post-v1.5.0
> ------------------------
> + - bonding
> + - LACP bonds no longer fall back to balance-slb when negotiations
> fail.
> + Instead they drop traffic.
>
>
> v1.5.0 - xx xxx xxxx
> diff --git a/lib/bond.c b/lib/bond.c
> index a2105ca..50a1d5d 100644
> --- a/lib/bond.c
> +++ b/lib/bond.c
> @@ -26,6 +26,7 @@
> #include "dynamic-string.h"
> #include "flow.h"
> #include "hmap.h"
> +#include "lacp.h"
> #include "list.h"
> #include "netdev.h"
> #include "odp-util.h"
> @@ -92,7 +93,7 @@ struct bond {
> struct bond_slave *active_slave;
> tag_type no_slaves_tag; /* Tag for flows when all slaves disabled. */
> int updelay, downdelay; /* Delay before slave goes up/down, in ms. */
> - bool lacp_negotiated; /* LACP negotiations were successful. */
> + enum lacp_status lacp_status; /* Status of LACP negotiations. */
> bool bond_revalidate; /* True if flows need revalidation. */
> uint32_t basis; /* Basis for flow hash function. */
>
> @@ -122,7 +123,6 @@ static void bond_enable_slave(struct bond_slave *, bool
> enable,
> struct tag_set *);
> static void bond_link_status_update(struct bond_slave *, struct tag_set *);
> static void bond_choose_active_slave(struct bond *, struct tag_set *);
> -static bool bond_is_tcp_hash(const struct bond *);
> static unsigned int bond_hash_src(const uint8_t mac[ETH_ADDR_LEN],
> uint16_t vlan, uint32_t basis);
> static unsigned int bond_hash_tcp(const struct flow *, uint16_t vlan,
> @@ -402,13 +402,12 @@ bond_slave_set_may_enable(struct bond *bond, void
> *slave_, bool may_enable)
> *
> * The caller should check bond_should_send_learning_packets() afterward. */
> void
> -bond_run(struct bond *bond, struct tag_set *tags, bool lacp_negotiated)
> +bond_run(struct bond *bond, struct tag_set *tags, enum lacp_status
> lacp_status)
> {
> struct bond_slave *slave;
> - bool is_tcp_hash = bond_is_tcp_hash(bond);
>
> - if (bond->lacp_negotiated != lacp_negotiated) {
> - bond->lacp_negotiated = lacp_negotiated;
> + if (bond->lacp_status != lacp_status) {
> + bond->lacp_status = lacp_status;
> bond->bond_revalidate = true;
> }
>
> @@ -427,10 +426,6 @@ bond_run(struct bond *bond, struct tag_set *tags, bool
> lacp_negotiated)
> bond->next_fake_iface_update = time_msec() + 1000;
> }
>
> - if (is_tcp_hash != bond_is_tcp_hash(bond)) {
> - bond->bond_revalidate = true;
> - }
> -
> if (bond->bond_revalidate) {
> bond->bond_revalidate = false;
>
> @@ -488,8 +483,9 @@ bond_wait(struct bond *bond)
> static bool
> may_send_learning_packets(const struct bond *bond)
> {
> - return !bond->lacp_negotiated && bond->balance != BM_AB &&
> - bond->active_slave;
> + return bond->lacp_status == LACP_DISABLED
> + && bond->balance != BM_AB
> + && bond->active_slave;
> }
>
> /* Returns true if 'bond' needs the client to send out packets to assist with
> @@ -566,9 +562,14 @@ bond_check_admissibility(struct bond *bond, const void
> *slave_,
> * assume the remote switch is aware of the bond and will "do the right
> * thing". However, as a precaution we drop packets on disabled slaves
> * because no correctly implemented partner switch should be sending
> - * packets to them. */
> - if (bond->lacp_negotiated) {
> - return slave->enabled ? BV_ACCEPT : BV_DROP;
> + * packets to them.
> + *
> + * If LACP is configured, but LACP negotiations have been unsuccessful,
> we
> + * drop all incoming traffic. */
> + switch (bond->lacp_status) {
> + case LACP_NEGOTIATED: return slave->enabled ? BV_ACCEPT : BV_DROP;
> + case LACP_CONFIGURED: return BV_DROP;
> + case LACP_DISABLED: break;
> }
>
> /* Drop all multicast packets on inactive slaves. */
> @@ -595,10 +596,11 @@ bond_check_admissibility(struct bond *bond, const void
> *slave_,
> return BV_ACCEPT;
>
> case BM_TCP:
> - /* TCP balancing has degraded to SLB (otherwise the
> - * bond->lacp_negotiated check above would have processed this).
> - *
> - * Fall through. */
> + /* TCP balanced bonds require successful LACP negotiated. Based on
> the
> + * above check, LACP is off on this bond. Therfore, we drop all
> + * incoming traffic. */
> + return BV_DROP;
> +
> case BM_SLB:
> /* Drop all packets for which we have learned a different input port,
> * because we probably sent the packet on one slave and got it back
> on
> @@ -950,11 +952,6 @@ bond_print_details(struct ds *ds, const struct bond
> *bond)
> ds_put_format(ds, "bond_mode: %s\n",
> bond_mode_to_string(bond->balance));
>
> - if (bond->balance != BM_AB) {
> - ds_put_format(ds, "bond-hash-algorithm: %s\n",
> - bond_is_tcp_hash(bond) ? "balance-tcp" :
> "balance-slb");
> - }
> -
> ds_put_format(ds, "bond-hash-basis: %"PRIu32"\n", bond->basis);
>
> ds_put_format(ds, "updelay: %d ms\n", bond->updelay);
> @@ -965,8 +962,21 @@ bond_print_details(struct ds *ds, const struct bond
> *bond)
> bond->next_rebalance - time_msec());
> }
>
> - ds_put_format(ds, "lacp_negotiated: %s\n",
> - bond->lacp_negotiated ? "true" : "false");
> + ds_put_cstr(ds, "lacp_status: ");
> + switch (bond->lacp_status) {
> + case LACP_NEGOTIATED:
> + ds_put_cstr(ds, "negotiated\n");
> + break;
> + case LACP_CONFIGURED:
> + ds_put_cstr(ds, "configured\n");
> + break;
> + case LACP_DISABLED:
> + ds_put_cstr(ds, "off\n");
> + break;
> + default:
> + ds_put_cstr(ds, "<unknown>\n");
> + break;
> + }
>
> HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> shash_add(&slave_shash, slave->name, slave);
> @@ -1305,7 +1315,7 @@ bond_link_status_update(struct bond_slave *slave,
> struct tag_set *tags)
> VLOG_INFO_RL(&rl, "interface %s: will not be %s",
> slave->name, up ? "disabled" : "enabled");
> } else {
> - int delay = (bond->lacp_negotiated ? 0
> + int delay = (bond->lacp_status != LACP_DISABLED ? 0
> : up ? bond->updelay : bond->downdelay);
> slave->delay_expires = time_msec() + delay;
> if (delay) {
> @@ -1324,13 +1334,6 @@ bond_link_status_update(struct bond_slave *slave,
> struct tag_set *tags)
> }
> }
>
> -static bool
> -bond_is_tcp_hash(const struct bond *bond)
> -{
> - return (bond->balance == BM_TCP && bond->lacp_negotiated)
> - || bond->balance == BM_STABLE;
> -}
> -
> static unsigned int
> bond_hash_src(const uint8_t mac[ETH_ADDR_LEN], uint16_t vlan, uint32_t basis)
> {
> @@ -1352,9 +1355,9 @@ bond_hash_tcp(const struct flow *flow, uint16_t vlan,
> uint32_t basis)
> static unsigned int
> bond_hash(const struct bond *bond, const struct flow *flow, uint16_t vlan)
> {
> - assert(bond->balance != BM_AB);
> + assert(bond->balance == BM_TCP || bond->balance == BM_SLB);
>
> - return (bond_is_tcp_hash(bond)
> + return (bond->balance == BM_TCP
> ? bond_hash_tcp(flow, vlan, bond->basis)
> : bond_hash_src(flow->dl_src, vlan, bond->basis));
> }
> @@ -1381,7 +1384,7 @@ choose_stb_slave(const struct bond *bond, const struct
> flow *flow,
>
> best = NULL;
> best_hash = 0;
> - flow_hash = bond_hash(bond, flow, vlan);
> + flow_hash = bond_hash_tcp(flow, vlan, bond->basis);
> HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
> if (slave->enabled) {
> uint32_t hash;
> @@ -1403,14 +1406,26 @@ choose_output_slave(const struct bond *bond, const
> struct flow *flow,
> {
> struct bond_entry *e;
>
> + if (bond->lacp_status == LACP_CONFIGURED) {
> + /* LACP has been configured on this bond but negotiations were
> + * unsuccussful. Drop all traffic. */
> + return NULL;
> + }
> +
> switch (bond->balance) {
> case BM_AB:
> return bond->active_slave;
>
> case BM_STABLE:
> return choose_stb_slave(bond, flow, vlan);
> - case BM_SLB:
> +
> case BM_TCP:
> + if (bond->lacp_status != LACP_NEGOTIATED) {
> + /* Must have LACP negotiations for TCP balanced bonds. */
> + return NULL;
> + }
> + /* Fall Through. */
> + case BM_SLB:
> e = lookup_bond_entry(bond, flow, vlan);
> if (!e->slave || !e->slave->enabled) {
> e->slave = CONTAINER_OF(hmap_random_node(&bond->slaves),
> diff --git a/lib/bond.h b/lib/bond.h
> index 1958029..9eb1b8f 100644
> --- a/lib/bond.h
> +++ b/lib/bond.h
> @@ -26,6 +26,7 @@
> struct flow;
> struct netdev;
> struct ofpbuf;
> +enum lacp_status;
>
> /* How flows are balanced among bond slaves. */
> enum bond_mode {
> @@ -68,7 +69,7 @@ void bond_slave_register(struct bond *, void *slave_,
> void bond_slave_set_netdev(struct bond *, void *slave_, struct netdev *);
> void bond_slave_unregister(struct bond *, const void *slave);
>
> -void bond_run(struct bond *, struct tag_set *, bool lacp_negotiated);
> +void bond_run(struct bond *, struct tag_set *, enum lacp_status);
> void bond_wait(struct bond *);
>
> void bond_slave_set_may_enable(struct bond *, void *slave_, bool may_enable);
> diff --git a/lib/lacp.c b/lib/lacp.c
> index 244b86e..c51fac6 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -301,12 +301,17 @@ lacp_process_packet(struct lacp *lacp, const void
> *slave_,
> }
> }
>
> -/* Returns true if 'lacp' has successfully negotiated with its partner.
> False
> - * if 'lacp' is NULL. */
> -bool
> -lacp_negotiated(const struct lacp *lacp)
> +/* Returns the lacp_status of the given 'lacp' object (which may be NULL). */
> +enum lacp_status
> +lacp_status(const struct lacp *lacp)
> {
> - return lacp ? lacp->negotiated : false;
> + if (!lacp) {
> + return LACP_DISABLED;
> + } else if (lacp->negotiated) {
> + return LACP_NEGOTIATED;
> + } else {
> + return LACP_CONFIGURED;
> + }
> }
>
> /* Registers 'slave_' as subordinate to 'lacp'. This should be called at
> least
> @@ -384,12 +389,8 @@ lacp_slave_may_enable(const struct lacp *lacp, const
> void *slave_)
> struct slave *slave = slave_lookup(lacp, slave_);
>
> /* The slave may be enabled if it's attached to an aggregator and its
> - * partner is synchronized. The only exception is defaulted slaves.
> - * They are not required to have synchronized partners because they
> - * have no partners at all. They will only be attached if
> negotiations
> - * failed on all slaves in the bond. */
> - return slave->attached && (slave->partner.state & LACP_STATE_SYNC
> - || slave->status == LACP_DEFAULTED);
> + * partner is synchronized.*/
> + return slave->attached && (slave->partner.state & LACP_STATE_SYNC);
> } else {
> return true;
> }
> @@ -504,14 +505,13 @@ lacp_update_attached(struct lacp *lacp)
> HMAP_FOR_EACH (slave, node, &lacp->slaves) {
> struct lacp_info pri;
>
> - slave->attached = true;
> + slave->attached = false;
>
> /* XXX: In the future allow users to configure the expected system
> ID.
> * For now just special case loopback. */
> if (eth_addr_equals(slave->partner.sys_id, slave->lacp->sys_id)) {
> VLOG_WARN_RL(&rl, "slave %s: Loopback detected. Slave is "
> "connected to its own bond", slave->name);
> - slave->attached = false;
> continue;
> }
>
> @@ -519,6 +519,7 @@ lacp_update_attached(struct lacp *lacp)
> continue;
> }
>
> + slave->attached = true;
> slave_get_priority(slave, &pri);
>
> if (!lead || memcmp(&pri, &lead_pri, sizeof pri) < 0) {
> @@ -531,8 +532,7 @@ lacp_update_attached(struct lacp *lacp)
>
> if (lead) {
> HMAP_FOR_EACH (slave, node, &lacp->slaves) {
> - if (slave->status == LACP_DEFAULTED
> - || lead->partner.key != slave->partner.key
> + if (lead->partner.key != slave->partner.key
> || !eth_addr_equals(lead->partner.sys_id,
> slave->partner.sys_id)) {
> slave->attached = false;
> diff --git a/lib/lacp.h b/lib/lacp.h
> index 1d717d6..293fc45 100644
> --- a/lib/lacp.h
> +++ b/lib/lacp.h
> @@ -29,6 +29,12 @@ enum lacp_time {
> LACP_TIME_CUSTOM /* Nonstandard custom mode. */
> };
>
> +enum lacp_status {
> + LACP_NEGOTIATED, /* Successful LACP negotations. */
> + LACP_CONFIGURED, /* LACP is enabled but not negotiated.
> */
> + LACP_DISABLED /* LACP is not enabled. */
> +};
> +
> struct lacp_settings {
> char *name; /* Name (for debugging). */
> uint8_t id[ETH_ADDR_LEN]; /* System ID. Must be nonzero. */
> @@ -48,7 +54,7 @@ bool lacp_is_active(const struct lacp *);
>
> void lacp_process_packet(struct lacp *, const void *slave,
> const struct ofpbuf *packet);
> -bool lacp_negotiated(const struct lacp *);
> +enum lacp_status lacp_status(const struct lacp *);
>
> struct lacp_slave_settings {
> char *name; /* Name (for debugging). */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6ecf71b..34dc903 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1866,7 +1866,7 @@ bundle_run(struct ofbundle *bundle)
> }
>
> bond_run(bundle->bond, &bundle->ofproto->revalidate_set,
> - lacp_negotiated(bundle->lacp));
> + lacp_status(bundle->lacp));
> if (bond_should_send_learning_packets(bundle->bond)) {
> bundle_send_learning_packets(bundle);
> }
> diff --git a/tests/lacp.at b/tests/lacp.at
> index 16daa3d..947eb9c 100644
> --- a/tests/lacp.at
> +++ b/tests/lacp.at
> @@ -104,7 +104,7 @@ bond_mode: active-backup
> bond-hash-basis: 0
> updelay: 0 ms
> downdelay: 0 ms
> -lacp_negotiated: true
> +lacp_status: negotiated
>
> slave p1: disabled
> may_enable: false
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 585f678..dcc25d1 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -757,8 +757,7 @@
>
> <p>
> The following modes require the upstream switch to support 802.3ad
> with
> - successful LACP negotiation. If LACP negotiation fails then
> - <code>balance-slb</code> style flow hashing is used as a fallback:
> + successful LACP negotiation:
> </p>
>
> <dl>
> @@ -861,8 +860,9 @@
> connected to. <code>active</code> ports are allowed to initiate
> LACP
> negotiations. <code>passive</code> ports are allowed to
> participate
> in LACP negotiations initiated by a remote switch, but not allowed
> to
> - initiate such negotiations themselves. Defaults to
> <code>off</code>
> - if unset.
> + initiate such negotiations themselves. If LACP is enabled on a
> port
> + whose partner switch does not support LACP, the bond will be
> + disabled. Defaults to <code>off</code> if unset.
> </column>
>
> <column name="other_config" key="lacp-system-id">
> --
> 1.7.7.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
