bridge: Refactor the 'Instant' stats logic.
authorAlex Wang <alexw@nicira.com>
Thu, 3 Apr 2014 20:12:35 +0000 (13:12 -0700)
committerAlex Wang <alexw@nicira.com>
Mon, 28 Apr 2014 04:21:51 +0000 (21:21 -0700)
This commit refactors the 'Instant' stats related logic in bridge.c
by moving it into bridge_run().

This change brings the following effects:

1. bridge.c will wait on the global connectivity sequence number when
   there is no pending instant stats transaction.  and the main thread
   will no longer be waken up every 100 ms for 'Instant' stats check.
   the related overhead is eliminated.

2. the netdev's sequence number is used to avoid updating unchanged netdev
   status.  so, the update is more efficient.

Signed-off-by: Alex Wang <alexw@nicira.com>
Acked-by: Joe Stringer <joestringer@nicira.com>
vswitchd/bridge.c

index e35f34d..84e9ab8 100644 (file)
@@ -77,6 +77,7 @@ struct iface {
     char *name;                 /* Host network device name. */
     struct netdev *netdev;      /* Network device. */
     ofp_port_t ofp_port;        /* OpenFlow port number. */
+    uint64_t change_seq;
 
     /* These members are valid only within bridge_reconfigure(). */
     const char *type;           /* Usually same as cfg->type. */
@@ -160,6 +161,25 @@ static unsigned int idl_seqno;
 /* Track changes to port connectivity. */
 static uint64_t connectivity_seqno = LLONG_MIN;
 
+/* Status update to database.
+ *
+ * Some information in the database must be kept as up-to-date as possible to
+ * allow controllers to respond rapidly to network outages.  Those status are
+ * updated via the 'status_txn'.
+ *
+ * We use the global connectivity sequence number to detect the status change.
+ * Also, to prevent the status update from sending too much to the database,
+ * we check the return status of each update transaction and do not start new
+ * update if the previous transaction status is 'TXN_INCOMPLETE'.
+ *
+ * 'statux_txn' is NULL if there is no ongoing status update.
+ */
+static struct ovsdb_idl_txn *status_txn;
+
+/* When the status update transaction returns 'TXN_INCOMPLETE', should register a
+ * timeout in 'STATUS_CHECK_AGAIN_MSEC' to check again. */
+#define STATUS_CHECK_AGAIN_MSEC 100
+
 /* Each time this timer expires, the bridge fetches interface and mirror
  * statistics and pushes them into the database. */
 #define IFACE_STATS_INTERVAL (5 * 1000) /* In milliseconds. */
@@ -264,7 +284,8 @@ static void iface_configure_qos(struct iface *, const struct ovsrec_qos *);
 static void iface_configure_cfm(struct iface *);
 static void iface_refresh_cfm_stats(struct iface *);
 static void iface_refresh_stats(struct iface *);
-static void iface_refresh_status(struct iface *);
+static void iface_refresh_netdev_status(struct iface *);
+static void iface_refresh_ofproto_status(struct iface *);
 static bool iface_is_synthetic(const struct iface *);
 static ofp_port_t iface_get_requested_ofp_port(
     const struct ovsrec_interface *);
@@ -1546,7 +1567,7 @@ iface_create(struct bridge *br, const struct ovsrec_interface *iface_cfg,
 
     /* Populate initial status in database. */
     iface_refresh_stats(iface);
-    iface_refresh_status(iface);
+    iface_refresh_netdev_status(iface);
 
     /* Add bond fake iface if necessary. */
     if (port_is_bond_fake_iface(port)) {
@@ -1804,22 +1825,27 @@ dpid_from_hash(const void *data, size_t n)
 }
 
 static void
-iface_refresh_status(struct iface *iface)
+iface_refresh_netdev_status(struct iface *iface)
 {
     struct smap smap;
 
     enum netdev_features current;
-    int64_t bps;
-    int mtu;
-    int64_t mtu_64;
+    enum netdev_flags flags;
+    const char *link_state;
     uint8_t mac[ETH_ADDR_LEN];
-    int64_t ifindex64;
-    int error;
+    int64_t bps, mtu_64, ifindex64, link_resets;
+    int mtu, error;
 
     if (iface_is_synthetic(iface)) {
         return;
     }
 
+    if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
+        return;
+    }
+
+    iface->change_seq = netdev_get_change_seq(iface->netdev);
+
     smap_init(&smap);
 
     if (!netdev_get_status(iface->netdev, &smap)) {
@@ -1830,6 +1856,21 @@ iface_refresh_status(struct iface *iface)
 
     smap_destroy(&smap);
 
+    error = netdev_get_flags(iface->netdev, &flags);
+    if (!error) {
+        const char *state = flags & NETDEV_UP ? "up" : "down";
+
+        ovsrec_interface_set_admin_state(iface->cfg, state);
+    } else {
+        ovsrec_interface_set_admin_state(iface->cfg, NULL);
+    }
+
+    link_state = netdev_get_carrier(iface->netdev) ? "up" : "down";
+    ovsrec_interface_set_link_state(iface->cfg, link_state);
+
+    link_resets = netdev_get_carrier_resets(iface->netdev);
+    ovsrec_interface_set_link_resets(iface->cfg, &link_resets, 1);
+
     error = netdev_get_features(iface->netdev, &current, NULL, NULL, NULL);
     bps = !error ? netdev_features_to_bps(current, 0) : 0;
     if (bps) {
@@ -1869,6 +1910,36 @@ iface_refresh_status(struct iface *iface)
     ovsrec_interface_set_ifindex(iface->cfg, &ifindex64, 1);
 }
 
+static void
+iface_refresh_ofproto_status(struct iface *iface)
+{
+    struct smap smap;
+    int current, error;
+
+    if (iface_is_synthetic(iface)) {
+        return;
+    }
+
+    current = ofproto_port_is_lacp_current(iface->port->bridge->ofproto,
+                                           iface->ofp_port);
+    if (current >= 0) {
+        bool bl = current;
+        ovsrec_interface_set_lacp_current(iface->cfg, &bl, 1);
+    } else {
+        ovsrec_interface_set_lacp_current(iface->cfg, NULL, 0);
+    }
+
+    iface_refresh_cfm_stats(iface);
+
+    smap_init(&smap);
+    error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
+                                        iface->ofp_port, &smap);
+    if (error >= 0) {
+        ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+    }
+    smap_destroy(&smap);
+}
+
 /* Writes 'iface''s CFM statistics to the database. 'iface' must not be
  * synthetic. */
 static void
@@ -2182,144 +2253,6 @@ refresh_controller_status(void)
     ofproto_free_ofproto_controller_info(&info);
 }
 \f
-/* "Instant" stats.
- *
- * Some information in the database must be kept as up-to-date as possible to
- * allow controllers to respond rapidly to network outages.  We call these
- * statistics "instant" stats.
- *
- * We wish to update these statistics every INSTANT_INTERVAL_MSEC milliseconds,
- * assuming that they've changed.  The only means we have to determine whether
- * they have changed are:
- *
- *   - Try to commit changes to the database.  If nothing changed, then
- *     ovsdb_idl_txn_commit() returns TXN_UNCHANGED, otherwise some other
- *     value.
- *
- *   - instant_stats_run() is called late in the run loop, after anything that
- *     might change any of the instant stats.
- *
- * We use these two facts together to avoid waking the process up every
- * INSTANT_INTERVAL_MSEC whether there is any change or not.
- */
-
-/* Minimum interval between writing updates to the instant stats to the
- * database. */
-#define INSTANT_INTERVAL_MSEC 100
-
-/* Current instant stats database transaction, NULL if there is no ongoing
- * transaction. */
-static struct ovsdb_idl_txn *instant_txn;
-
-/* Next time (in msec on monotonic clock) at which we will update the instant
- * stats.  */
-static long long int instant_next_txn = LLONG_MIN;
-
-/* True if the run loop has run since we last saw that the instant stats were
- * unchanged, that is, this is true if we need to wake up at 'instant_next_txn'
- * to refresh the instant stats. */
-static bool instant_stats_could_have_changed;
-
-static void
-instant_stats_run(void)
-{
-    enum ovsdb_idl_txn_status status;
-
-    instant_stats_could_have_changed = true;
-
-    if (!instant_txn) {
-        struct bridge *br;
-        uint64_t seq;
-
-        if (time_msec() < instant_next_txn) {
-            return;
-        }
-        instant_next_txn = time_msec() + INSTANT_INTERVAL_MSEC;
-
-        seq = seq_read(connectivity_seq_get());
-        if (seq == connectivity_seqno) {
-            return;
-        }
-        connectivity_seqno = seq;
-
-        instant_txn = ovsdb_idl_txn_create(idl);
-        HMAP_FOR_EACH (br, node, &all_bridges) {
-            struct iface *iface;
-            struct port *port;
-
-            br_refresh_stp_status(br);
-
-            HMAP_FOR_EACH (port, hmap_node, &br->ports) {
-                port_refresh_stp_status(port);
-            }
-
-            HMAP_FOR_EACH (iface, name_node, &br->iface_by_name) {
-                enum netdev_flags flags;
-                struct smap smap;
-                const char *link_state;
-                int64_t link_resets;
-                int current, error;
-
-                if (iface_is_synthetic(iface)) {
-                    continue;
-                }
-
-                current = ofproto_port_is_lacp_current(br->ofproto,
-                                                       iface->ofp_port);
-                if (current >= 0) {
-                    bool bl = current;
-                    ovsrec_interface_set_lacp_current(iface->cfg, &bl, 1);
-                } else {
-                    ovsrec_interface_set_lacp_current(iface->cfg, NULL, 0);
-                }
-
-                error = netdev_get_flags(iface->netdev, &flags);
-                if (!error) {
-                    const char *state = flags & NETDEV_UP ? "up" : "down";
-                    ovsrec_interface_set_admin_state(iface->cfg, state);
-                } else {
-                    ovsrec_interface_set_admin_state(iface->cfg, NULL);
-                }
-
-                link_state = netdev_get_carrier(iface->netdev) ? "up" : "down";
-                ovsrec_interface_set_link_state(iface->cfg, link_state);
-
-                link_resets = netdev_get_carrier_resets(iface->netdev);
-                ovsrec_interface_set_link_resets(iface->cfg, &link_resets, 1);
-
-                iface_refresh_cfm_stats(iface);
-
-                smap_init(&smap);
-                error = ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
-                                                    &smap);
-                if (error >= 0) {
-                    ovsrec_interface_set_bfd_status(iface->cfg, &smap);
-                }
-                smap_destroy(&smap);
-            }
-        }
-    }
-
-    status = ovsdb_idl_txn_commit(instant_txn);
-    if (status != TXN_INCOMPLETE) {
-        ovsdb_idl_txn_destroy(instant_txn);
-        instant_txn = NULL;
-    }
-    if (status == TXN_UNCHANGED) {
-        instant_stats_could_have_changed = false;
-    }
-}
-
-static void
-instant_stats_wait(void)
-{
-    if (instant_txn) {
-        ovsdb_idl_txn_wait(instant_txn);
-    } else if (instant_stats_could_have_changed) {
-        poll_timer_wait_until(instant_next_txn);
-    }
-}
-\f
 static void
 bridge_run__(void)
 {
@@ -2469,7 +2402,6 @@ bridge_run(void)
 
                     LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
                         iface_refresh_stats(iface);
-                        iface_refresh_status(iface);
                     }
 
                     port_refresh_stp_stats(port);
@@ -2488,8 +2420,44 @@ bridge_run(void)
         iface_stats_timer = time_msec() + IFACE_STATS_INTERVAL;
     }
 
+    if (!status_txn) {
+        uint64_t seq;
+
+        /* Check the need to update status. */
+        seq = seq_read(connectivity_seq_get());
+        if (seq != connectivity_seqno) {
+            connectivity_seqno = seq;
+            status_txn = ovsdb_idl_txn_create(idl);
+            HMAP_FOR_EACH (br, node, &all_bridges) {
+                struct port *port;
+
+                br_refresh_stp_status(br);
+                HMAP_FOR_EACH (port, hmap_node, &br->ports) {
+                    struct iface *iface;
+
+                    port_refresh_stp_status(port);
+                    LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
+                        iface_refresh_netdev_status(iface);
+                        iface_refresh_ofproto_status(iface);
+                    }
+                }
+            }
+        }
+    }
+
+    if (status_txn) {
+        enum ovsdb_idl_txn_status status;
+
+        status = ovsdb_idl_txn_commit(status_txn);
+        /* Do not destroy "status_txn" if the transaction is
+         * "TXN_INCOMPLETE". */
+        if (status != TXN_INCOMPLETE) {
+            ovsdb_idl_txn_destroy(status_txn);
+            status_txn = NULL;
+        }
+    }
+
     run_system_stats();
-    instant_stats_run();
 }
 
 void
@@ -2519,8 +2487,17 @@ bridge_wait(void)
         poll_timer_wait_until(iface_stats_timer);
     }
 
+    /* If the status database transaction is 'TXN_INCOMPLETE' in this run,
+     * register a timeout in 'STATUS_CHECK_AGAIN_MSEC'.  Else, wait on the
+     * global connectivity sequence number.  Note, this also helps batch
+     * multiple status changes into one transaction. */
+    if (status_txn) {
+        poll_timer_wait_until(time_msec() + STATUS_CHECK_AGAIN_MSEC);
+    } else {
+        seq_wait(connectivity_seq_get(), connectivity_seqno);
+    }
+
     system_stats_wait();
-    instant_stats_wait();
 }
 
 /* Adds some memory usage statistics for bridges into 'usage', for use with