bond: Give bridge control over LACP module.

Before this patch, the bonding code had taken over responsibility
for running the LACP module.  However, the bonding code only needs
the LACP module for some basic status queries.  LACP and bonding
are actually logically parallel modules and do not really have a
parent child relationship.  Furthermore, we need to be able to run
LACP on non-bonded interfaces which the existing approach
prevented.  This patch gives control of the LACP module back to the
bridge.

diff --git a/lib/bond.c b/lib/bond.c
index 9f857e9d4b272d6101caeb1586c8a174af3f96c5..3ec1ba2075ed793225cd6b7f1045397d70b0d4fe 100644 (file)
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -40,8 +40,6 @@
 
 VLOG_DEFINE_THIS_MODULE(bond);
 
-COVERAGE_DEFINE(bond_process_lacp);
-
 /* Bit-mask for hashing a flow down to a bucket.
  * There are (BOND_MASK + 1) buckets. */
 #define BOND_MASK 0xff
@@ -109,7 +107,7 @@ struct bond {
     bool stb_need_sort;             /* True if stb_slaves is not sorted. */
 
     /* LACP. */
-    struct lacp *lacp;          /* LACP object. NULL if LACP is disabled. */
+    const struct lacp *lacp;        /* Client provided LACP handle. */
 
     /* Monitoring. */
     enum bond_detect_mode detect;     /* Link status mode, one of BLSM_*. */
@@ -260,8 +258,6 @@ bond_destroy(struct bond *bond)
 
     free(bond->hash);
 
-    lacp_destroy(bond->lacp);
-
     netdev_monitor_destroy(bond->monitor);
 
     free(bond->name);
@@ -293,6 +289,7 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
         hmap_insert(&all_bonds, &bond->hmap_node, hash_string(bond->name, 0));
     }
 
+    bond->lacp = s->lacp;
     bond->detect = s->detect;
     bond->miimon_interval = s->miimon_interval;
     bond->updelay = s->up_delay;
@@ -323,16 +320,6 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
         }
     }
 
-    if (s->lacp) {
-        if (!bond->lacp) {
-            bond->lacp = lacp_create();
-        }
-        lacp_configure(bond->lacp, s->lacp);
-    } else {
-        lacp_destroy(bond->lacp);
-        bond->lacp = NULL;
-    }
-
     if (s->fake_iface) {
         if (bond->next_fake_iface_update == LLONG_MAX) {
             bond->next_fake_iface_update = time_msec();
@@ -374,12 +361,10 @@ bond_reconfigure(struct bond *bond, const struct bond_settings *s)
  * by the client, so the client must not close it before either unregistering
  * 'slave_' or destroying 'bond'.
  *
- * If 'bond' has a LACP configuration then 'lacp_settings' must point to LACP
- * settings for 'slave_'; otherwise 'lacp_settings' is ignored.
- */
+ * If 'bond' has a LACP configuration then 'slave_' should be the same handle
+ * used in the LACP module. */
 void
-bond_slave_register(struct bond *bond, void *slave_, struct netdev *netdev,
-                    const struct lacp_slave_settings *lacp_settings)
+bond_slave_register(struct bond *bond, void *slave_, struct netdev *netdev)
 {
     struct bond_slave *slave = bond_slave_lookup(bond, slave_);
 
@@ -398,11 +383,6 @@ bond_slave_register(struct bond *bond, void *slave_, struct netdev *netdev,
     slave->netdev = netdev;
     free(slave->name);
     slave->name = xstrdup(netdev_get_name(netdev));
-
-    if (bond->lacp) {
-        assert(lacp_settings != NULL);
-        lacp_slave_register(bond->lacp, slave, lacp_settings);
-    }
 }
 
 /* Unregisters 'slave_' from 'bond'.  If 'bond' does not contain such a slave
@@ -446,38 +426,6 @@ bond_slave_unregister(struct bond *bond, const void *slave_)
     }
 }
 
-/* Callback for lacp_run(). */
-static void
-bond_send_pdu_cb(void *slave_, const struct lacp_pdu *pdu)
-{
-    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 10);
-    struct bond_slave *slave = slave_;
-    uint8_t ea[ETH_ADDR_LEN];
-    int error;
-
-    error = netdev_get_etheraddr(slave->netdev, ea);
-    if (!error) {
-        struct lacp_pdu *packet_pdu;
-        struct ofpbuf packet;
-
-        ofpbuf_init(&packet, 0);
-        packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP,
-                                 sizeof *packet_pdu);
-        *packet_pdu = *pdu;
-        error = netdev_send(slave->netdev, &packet);
-        if (error) {
-            VLOG_WARN_RL(&rl, "bond %s: sending LACP PDU on slave %s failed "
-                         "(%s)",
-                         slave->bond->name, slave->name, strerror(error));
-        }
-        ofpbuf_uninit(&packet);
-    } else {
-        VLOG_ERR_RL(&rl, "bond %s: cannot obtain Ethernet address of slave "
-                    "%s (%s)",
-                    slave->bond->name, slave->name, strerror(error));
-    }
-}
-
 /* Performs periodic maintenance on 'bond'.  The caller must provide 'tags' to
  * allow tagged flows to be invalidated.
  *
@@ -498,11 +446,6 @@ bond_run(struct bond *bond, struct tag_set *tags)
         bond->miimon_next_update = time_msec() + bond->miimon_interval;
     }
 
-    /* Update LACP. */
-    if (bond->lacp) {
-        lacp_run(bond->lacp, bond_send_pdu_cb);
-    }
-
     /* Enable slaves based on link status and LACP feedback. */
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         bond_link_status_update(slave, tags);
@@ -691,24 +634,6 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow,
         return NULL;
     }
 }
-
-/* Processes LACP packet 'packet', which was received on 'slave_' within
- * 'bond'.
- *
- * The client should use this function to pass along LACP messages received on
- * any of 'bond''s slaves. */
-void
-bond_process_lacp(struct bond *bond, void *slave_, const struct ofpbuf *packet)
-{
-    if (bond->lacp) {
-        struct bond_slave *slave = bond_slave_lookup(bond, slave_);
-        const struct lacp_pdu *pdu = parse_lacp_packet(packet);
-        if (slave && pdu) {
-            COVERAGE_INC(bond_process_lacp);
-            lacp_process_pdu(bond->lacp, slave, pdu);
-        }
-    }
-}
 \f
 /* Rebalancing. */
 
@@ -1288,8 +1213,6 @@ bond_unixctl_hash(struct unixctl_conn *conn, const char *args_,
 void
 bond_init(void)
 {
-    lacp_init();
-
     unixctl_command_register("bond/list", bond_unixctl_list, NULL);
     unixctl_command_register("bond/show", bond_unixctl_show, NULL);
     unixctl_command_register("bond/migrate", bond_unixctl_migrate, NULL);
@@ -1350,12 +1273,12 @@ bond_stb_sort_cmp__(const void *a_, const void *b_)
     const struct bond_slave *const *bp = b_;
     const struct bond_slave *a = *ap;
     const struct bond_slave *b = *bp;
-    struct lacp *lacp = a->bond->lacp;
+    const struct lacp *lacp = a->bond->lacp;
     int a_id, b_id;
 
     if (lacp) {
-        a_id = lacp_slave_get_port_id(lacp, a);
-        b_id = lacp_slave_get_port_id(lacp, b);
+        a_id = lacp_slave_get_port_id(lacp, a->aux);
+        b_id = lacp_slave_get_port_id(lacp, b->aux);
     } else {
         a_id = netdev_get_ifindex(a->netdev);
         b_id = netdev_get_ifindex(b->netdev);
@@ -1436,7 +1359,7 @@ bond_link_status_update(struct bond_slave *slave, struct tag_set *tags)
     struct bond *bond = slave->bond;
     bool up;
 
-    up = slave->up && lacp_slave_may_enable(bond->lacp, slave);
+    up = slave->up && lacp_slave_may_enable(bond->lacp, slave->aux);
     if ((up == slave->enabled) != (slave->delay_expires == LLONG_MAX)) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
         VLOG_INFO_RL(&rl, "interface %s: link state %s",
@@ -1560,7 +1483,7 @@ bond_choose_slave(const struct bond *bond)
     best = NULL;
     HMAP_FOR_EACH (slave, hmap_node, &bond->slaves) {
         if (slave->delay_expires != LLONG_MAX
-            && lacp_slave_may_enable(bond->lacp, slave)
+            && lacp_slave_may_enable(bond->lacp, slave->aux)
             && (!best || slave->delay_expires < best->delay_expires)) {
             best = slave;
         }

diff --git a/lib/bond.h b/lib/bond.h
index 1e8ea7e12d850692f0ae32a716012f543f06a254..cb6563af15ce3cb4ee399067475d3e0a53972d6e 100644 (file)
--- a/lib/bond.h
+++ b/lib/bond.h
@@ -24,7 +24,6 @@
 #include "tag.h"
 
 struct flow;
-struct lacp_slave_settings;
 struct netdev;
 struct ofpbuf;
 
@@ -65,8 +64,7 @@ struct bond_settings {
     /* Legacy compatibility. */
     bool fake_iface;            /* Update fake stats for netdev 'name'? */
 
-    /* LACP. */
-    struct lacp_settings *lacp; /* NULL to disable LACP. */
+    const struct lacp *lacp;    /* LACP for this bond.  May be NULL. */
 };
 
 /* Program startup. */
@@ -77,8 +75,7 @@ struct bond *bond_create(const struct bond_settings *);
 void bond_destroy(struct bond *);
 
 bool bond_reconfigure(struct bond *, const struct bond_settings *);
-void bond_slave_register(struct bond *, void *slave_, struct netdev *,
-                         const struct lacp_slave_settings *);
+void bond_slave_register(struct bond *, void *slave_, struct netdev *);
 void bond_slave_unregister(struct bond *, const void *slave);
 
 void bond_run(struct bond *, struct tag_set *);
@@ -101,8 +98,6 @@ enum bond_verdict bond_check_admissibility(struct bond *, const void *slave_,
                                            tag_type *);
 void *bond_choose_output_slave(struct bond *,
                                const struct flow *, uint16_t vlan, tag_type *);
-void bond_process_lacp(struct bond *, void *slave,
-                       const struct ofpbuf *packet);
 
 /* Rebalancing. */
 void bond_account(struct bond *, const struct flow *, uint16_t vlan,

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 26b4f707524a5505f65544603a829c9de0272166..9b918af5e371c89c9c55a4e5c422884d66367a0a 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -147,6 +147,8 @@ struct port {
      * A bridge port for bonding has at least 2 interfaces. */
     struct list ifaces;         /* List of "struct iface"s. */
 
+    struct lacp *lacp;          /* NULL if LACP is not enabled. */
+
     /* Bonding info. */
     struct bond *bond;
 
@@ -238,6 +240,7 @@ static struct port *port_lookup(const struct bridge *, const char *name);
 static struct iface *port_get_an_iface(const struct port *);
 static struct port *port_from_dp_ifidx(const struct bridge *,
                                        uint16_t dp_ifidx);
+static void port_reconfigure_lacp(struct port *);
 static void port_reconfigure_bond(struct port *);
 static void port_send_learning_packets(struct port *);
 
@@ -339,6 +342,7 @@ bridge_init(const char *remote)
                              NULL);
     unixctl_command_register("bridge/reconnect", bridge_unixctl_reconnect,
                              NULL);
+    lacp_init();
     bond_init();
 }
 
@@ -882,6 +886,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
         HMAP_FOR_EACH (port, hmap_node, &br->ports) {
             struct iface *iface;
 
+            port_reconfigure_lacp(port);
             port_reconfigure_bond(port);
 
             LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
@@ -2630,8 +2635,11 @@ bridge_special_ofhook_cb(const struct flow *flow,
     iface = iface_from_dp_ifidx(br, flow->in_port);
 
     if (flow->dl_type == htons(ETH_TYPE_LACP)) {
-        if (iface && iface->port->bond && packet) {
-            bond_process_lacp(iface->port->bond, iface, packet);
+        if (iface && iface->port->lacp && packet) {
+            const struct lacp_pdu *pdu = parse_lacp_packet(packet);
+            if (pdu) {
+                lacp_process_pdu(iface->port->lacp, iface, pdu);
+            }
         }
         return false;
     }
@@ -2729,9 +2737,44 @@ static struct ofhooks bridge_ofhooks = {
 \f
 /* Port functions. */
 
+static void
+lacp_send_pdu_cb(void *iface_, const struct lacp_pdu *pdu)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 10);
+    struct iface *iface = iface_;
+    uint8_t ea[ETH_ADDR_LEN];
+    int error;
+
+    error = netdev_get_etheraddr(iface->netdev, ea);
+    if (!error) {
+        struct lacp_pdu *packet_pdu;
+        struct ofpbuf packet;
+
+        ofpbuf_init(&packet, 0);
+        packet_pdu = eth_compose(&packet, eth_addr_lacp, ea, ETH_TYPE_LACP,
+                                 sizeof *packet_pdu);
+        *packet_pdu = *pdu;
+        error = netdev_send(iface->netdev, &packet);
+        if (error) {
+            VLOG_WARN_RL(&rl, "port %s: sending LACP PDU on iface %s failed "
+                         "(%s)", iface->port->name, iface->name,
+                         strerror(error));
+        }
+        ofpbuf_uninit(&packet);
+    } else {
+        VLOG_ERR_RL(&rl, "port %s: cannot obtain Ethernet address of iface "
+                    "%s (%s)", iface->port->name, iface->name,
+                    strerror(error));
+    }
+}
+
 static void
 port_run(struct port *port)
 {
+    if (port->lacp) {
+        lacp_run(port->lacp, lacp_send_pdu_cb);
+    }
+
     if (port->bond) {
         bond_run(port->bond,
                  ofproto_get_revalidate_set(port->bridge->ofproto));
@@ -2744,6 +2787,10 @@ port_run(struct port *port)
 static void
 port_wait(struct port *port)
 {
+    if (port->lacp) {
+        lacp_wait(port->lacp);
+    }
+
     if (port->bond) {
         bond_wait(port->bond);
     }
@@ -2966,6 +3013,7 @@ port_destroy(struct port *port)
 
         VLOG_INFO("destroyed port %s on bridge %s", port->name, br->name);
 
+        lacp_destroy(port->lacp);
         port_flush_macs(port);
 
         bitmap_free(port->trunks);
@@ -3017,29 +3065,8 @@ enable_lacp(struct port *port, bool *activep)
     }
 }
 
-static struct lacp_settings *
-port_reconfigure_bond_lacp(struct port *port, struct lacp_settings *s)
-{
-    if (!enable_lacp(port, &s->active)) {
-        return NULL;
-    }
-
-    s->name = port->name;
-    memcpy(s->id, port->bridge->ea, ETH_ADDR_LEN);
-    s->priority = atoi(get_port_other_config(port->cfg, "lacp-system-priority",
-                                             "0"));
-    s->fast = !strcmp(get_port_other_config(port->cfg, "lacp-time", "slow"),
-                      "fast");
-
-    if (s->priority <= 0 || s->priority > UINT16_MAX) {
-        /* Prefer bondable links if unspecified. */
-        s->priority = UINT16_MAX - !list_is_short(&port->ifaces);
-    }
-    return s;
-}
-
 static void
-iface_reconfigure_bond(struct iface *iface)
+iface_reconfigure_lacp(struct iface *iface)
 {
     struct lacp_slave_settings s;
     int priority;
@@ -3050,13 +3077,47 @@ iface_reconfigure_bond(struct iface *iface)
                         iface->cfg, "lacp-port-priority", "0"));
     s.priority = (priority >= 0 && priority <= UINT16_MAX
                   ? priority : UINT16_MAX);
-    bond_slave_register(iface->port->bond, iface, iface->netdev, &s);
+    lacp_slave_register(iface->port->lacp, iface, &s);
+}
+
+static void
+port_reconfigure_lacp(struct port *port)
+{
+    static struct lacp_settings s;
+    struct iface *iface;
+
+    if (!enable_lacp(port, &s.active)) {
+        lacp_destroy(port->lacp);
+        port->lacp = NULL;
+        return;
+    }
+
+    s.name = port->name;
+    memcpy(s.id, port->bridge->ea, ETH_ADDR_LEN);
+    s.priority = atoi(get_port_other_config(port->cfg, "lacp-system-priority",
+                                            "0"));
+    s.fast = !strcmp(get_port_other_config(port->cfg, "lacp-time", "slow"),
+                     "fast");
+
+    if (s.priority <= 0 || s.priority > UINT16_MAX) {
+        /* Prefer bondable links if unspecified. */
+        s.priority = UINT16_MAX - !list_is_short(&port->ifaces);
+    }
+
+    if (!port->lacp) {
+        port->lacp = lacp_create();
+    }
+
+    lacp_configure(port->lacp, &s);
+
+    LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
+        iface_reconfigure_lacp(iface);
+    }
 }
 
 static void
 port_reconfigure_bond(struct port *port)
 {
-    struct lacp_settings lacp_settings;
     struct bond_settings s;
     const char *detect_s;
     struct iface *iface;
@@ -3102,7 +3163,7 @@ port_reconfigure_bond(struct port *port)
     }
 
     s.fake_iface = port->cfg->bond_fake_iface;
-    s.lacp = port_reconfigure_bond_lacp(port, &lacp_settings);
+    s.lacp = port->lacp;
 
     if (!port->bond) {
         port->bond = bond_create(&s);
@@ -3113,7 +3174,7 @@ port_reconfigure_bond(struct port *port)
     }
 
     LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
-        iface_reconfigure_bond(iface);
+        bond_slave_register(iface->port->bond, iface, iface->netdev);
     }
 }
 
@@ -3186,6 +3247,10 @@ iface_destroy(struct iface *iface)
             bond_slave_unregister(port->bond, iface);
         }
 
+        if (port->lacp) {
+            lacp_slave_unregister(port->lacp, iface);
+        }
+
         shash_find_and_delete_assert(&br->iface_by_name, iface->name);
 
         if (iface->dp_ifidx >= 0) {